pull/17362/merge
David 2 days ago committed by GitHub
commit f47b901d2a
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194

@ -0,0 +1,5 @@
---
'svelte': patch
---
fix: performance regression affecting deriveds with no dependencies (#17342)

@ -0,0 +1,5 @@
---
'svelte': patch
---
fix: reconnect deriveds inside branch effects

@ -28,9 +28,9 @@ import {
is_dirty,
is_updating_effect,
set_is_updating_effect,
set_signal_status,
update_effect
} from '../runtime.js';
import { set_signal_status } from './status.js';
import * as e from '../errors.js';
import { flush_tasks, queue_micro_task } from '../dom/task.js';
import { DEV } from 'esm-env';

@ -17,13 +17,13 @@ import {
import {
active_reaction,
active_effect,
set_signal_status,
update_reaction,
increment_write_version,
set_active_effect,
push_reaction_value,
is_destroying_effect
} from '../runtime.js';
import { set_signal_status, update_derived_status } from './status.js';
import { equals, safe_equals } from './equality.js';
import * as e from '../errors.js';
import * as w from '../warnings.js';
@ -360,7 +360,10 @@ export function update_derived(derived) {
// the underlying value will be updated when the fork is committed.
// otherwise, the next time we get here after a 'real world' state
// change, `derived.equals` may incorrectly return `true`
if (!current_batch?.is_fork) {
//
// deriveds with no deps should always update `derived.v`
// since they will never change and need the value after fork commits
if (!current_batch?.is_fork || derived.deps === null) {
derived.v = value;
}
@ -381,8 +384,9 @@ export function update_derived(derived) {
if (effect_tracking() || current_batch?.is_fork) {
batch_values.set(derived, value);
}
} else {
var status = (derived.f & CONNECTED) === 0 ? MAYBE_DIRTY : CLEAN;
set_signal_status(derived, status);
}
if (batch_values === null || derived.deps === null) {
update_derived_status(derived);
}
}

@ -1,4 +1,5 @@
/** @import { ComponentContext, ComponentContextLegacy, Derived, Effect, TemplateNode, TransitionManager } from '#client' */
import { set_signal_status } from './status.js';
import {
is_dirty,
active_effect,
@ -9,7 +10,6 @@ import {
remove_reactions,
set_active_reaction,
set_is_destroying_effect,
set_signal_status,
untrack,
untracking
} from '../runtime.js';

@ -6,7 +6,6 @@ import {
untracked_writes,
get,
set_untracked_writes,
set_signal_status,
untrack,
increment_write_version,
update_effect,
@ -18,6 +17,7 @@ import {
set_is_updating_effect,
is_updating_effect
} from '../runtime.js';
import { set_signal_status, update_derived_status } from './status.js';
import { equals, safe_equals } from './equality.js';
import {
CLEAN,
@ -218,12 +218,14 @@ export function internal_set(source, value) {
}
if ((source.f & DERIVED) !== 0) {
const derived = /** @type {Derived} */ (source);
// if we are assigning to a dirty derived we set it to clean/maybe dirty but we also eagerly execute it to track the dependencies
if ((source.f & DIRTY) !== 0) {
execute_derived(/** @type {Derived} */ (source));
execute_derived(derived);
}
set_signal_status(source, (source.f & CONNECTED) !== 0 ? CLEAN : MAYBE_DIRTY);
update_derived_status(derived);
}
source.wv = increment_write_version();

@ -0,0 +1,25 @@
/** @import { Derived, Signal } from '#client' */
import { CLEAN, CONNECTED, DIRTY, MAYBE_DIRTY } from '#client/constants';
const STATUS_MASK = ~(DIRTY | MAYBE_DIRTY | CLEAN);
/**
* @param {Signal} signal
* @param {number} status
*/
export function set_signal_status(signal, status) {
signal.f = (signal.f & STATUS_MASK) | status;
}
/**
* Set a derived's status to CLEAN or MAYBE_DIRTY based on its connection state.
* @param {Derived} derived
*/
export function update_derived_status(derived) {
// Only mark as MAYBE_DIRTY if disconnected and has dependencies.
if ((derived.f & CONNECTED) !== 0 || derived.deps === null) {
set_signal_status(derived, CLEAN);
} else {
set_signal_status(derived, MAYBE_DIRTY);
}
}

@ -28,7 +28,6 @@ import { old_values } from './reactivity/sources.js';
import {
destroy_derived_effects,
execute_derived,
current_async_effect,
recent_async_deriveds,
update_derived
} from './reactivity/deriveds.js';
@ -56,6 +55,7 @@ import { handle_error } from './error-handling.js';
import { UNINITIALIZED } from '../../constants.js';
import { captured_signals } from './legacy.js';
import { without_reactive_context } from './dom/elements/bindings/shared.js';
import { set_signal_status, update_derived_status } from './reactivity/status.js';
export let is_updating_effect = false;
@ -186,12 +186,12 @@ export function is_dirty(reaction) {
}
if (
(flags & CONNECTED) !== 0 &&
(flags & DERIVED) !== 0 &&
// During time traveling we don't want to reset the status so that
// traversal of the graph in the other batches still happens
batch_values === null
(batch_values === null || reaction.deps === null)
) {
set_signal_status(reaction, CLEAN);
update_derived_status(/** @type {Derived} */ (reaction));
}
}
@ -629,7 +629,14 @@ export function get(signal) {
update_derived(derived);
}
if (is_updating_effect && effect_tracking() && (derived.f & CONNECTED) === 0) {
if (
(derived.f & CONNECTED) === 0 &&
((is_updating_effect &&
(effect_tracking() ||
(active_effect !== null && (active_effect.f & BRANCH_EFFECT) !== 0))) ||
// evaluating connected parent derived, so reconnect child deriveds too
(active_reaction !== null && (active_reaction.f & CONNECTED) !== 0))
) {
reconnect(derived);
}
}
@ -718,17 +725,6 @@ export function untrack(fn) {
}
}
const STATUS_MASK = ~(DIRTY | MAYBE_DIRTY | CLEAN);
/**
* @param {Signal} signal
* @param {number} status
* @returns {void}
*/
export function set_signal_status(signal, status) {
signal.f = (signal.f & STATUS_MASK) | status;
}
/**
* @param {Record<string | symbol, unknown>} obj
* @param {Array<string | symbol>} keys

@ -3,7 +3,8 @@ import { DIRTY, LEGACY_PROPS, MAYBE_DIRTY } from '../internal/client/constants.j
import { user_pre_effect } from '../internal/client/reactivity/effects.js';
import { mutable_source, set } from '../internal/client/reactivity/sources.js';
import { hydrate, mount, unmount } from '../internal/client/render.js';
import { active_effect, get, set_signal_status } from '../internal/client/runtime.js';
import { active_effect, get } from '../internal/client/runtime.js';
import { set_signal_status } from '../internal/client/reactivity/status.js';
import { flushSync } from '../internal/client/reactivity/batch.js';
import { define_property, is_array } from '../internal/shared/utils.js';
import * as e from '../internal/client/errors.js';

@ -0,0 +1,10 @@
<script>
class Y {
foo = $state(0);
}
let y = $derived(new Y());
</script>
<button onclick={() => y.foo++}>click</button>
<p>{y.foo}</p>

@ -0,0 +1,24 @@
import { flushSync } from 'svelte';
import { test } from '../../test';
export default test({
skip_no_async: true,
async test({ assert, target }) {
const forkButton = target.querySelector('button');
flushSync(() => {
forkButton?.click();
});
const [, clickButton] = target.querySelectorAll('button');
const p = target.querySelector('p');
assert.equal(p?.textContent, '0');
flushSync(() => {
clickButton?.click();
});
assert.equal(p?.textContent, '1');
}
});

@ -0,0 +1,17 @@
<script>
import { fork } from 'svelte';
import Child from './Child.svelte';
let x = $state(false);
</script>
<button onclick={() => {
const f = fork(() => {
x = true;
});
f.commit();
}}>fork</button>
{#if x}
<Child />
{/if}

@ -1,3 +1,4 @@
/** @import { Reaction } from '#client' */
import { describe, assert, it } from 'vitest';
import { flushSync } from '../../src/index-client';
import * as $ from '../../src/internal/client/runtime';
@ -15,9 +16,10 @@ import { proxy } from '../../src/internal/client/proxy';
import { derived } from '../../src/internal/client/reactivity/deriveds';
import { snapshot } from '../../src/internal/shared/clone.js';
import { SvelteSet } from '../../src/reactivity/set';
import { DESTROYED } from '../../src/internal/client/constants';
import { CLEAN, CONNECTED, DESTROYED, MAYBE_DIRTY } from '../../src/internal/client/constants';
import { noop } from 'svelte/internal/client';
import { disable_async_mode_flag, enable_async_mode_flag } from '../../src/internal/flags';
import { branch } from '../../src/internal/client/reactivity/effects';
/**
* @param runes runes mode
@ -1493,4 +1495,159 @@ describe('signals', () => {
assert.deepEqual(log, ['inner destroyed', 'inner destroyed']);
};
});
// reproduces conditions leading to https://github.com/sveltejs/kit/issues/15059
test('derived read inside branch effect should be reconnected even when effect_tracking is false', () => {
let count = state(0);
let d: Derived<number> | null = null;
render_effect(() => {
branch(() => {
if (!d) {
d = derived(() => $.get(count) * 2);
}
$.get(d);
});
});
return () => {
flushSync();
const isConnected = (d!.f & CONNECTED) !== 0;
assert.ok(
isConnected,
'derived should be CONNECTED after being read inside branch during effect update'
);
const countReactions = count.reactions || [];
assert.ok(countReactions.includes(d!), 'derived should be in source reactions');
};
});
// Test that deriveds with no dependencies are always CLEAN
test('deriveds with no deps should be CLEAN and not re-evaluate', () => {
let evalCount = 0;
let d: Derived<number> | null = null;
render_effect(() => {
branch(() => {
if (!d) {
d = derived(() => {
evalCount++;
return 42;
});
}
$.get(d);
});
});
return () => {
flushSync();
const initialEvalCount = evalCount;
assert.equal(initialEvalCount, 1, 'derived should evaluate once initially');
for (let i = 0; i < 100; i++) {
$.get(d!);
}
assert.equal(
evalCount,
initialEvalCount,
'derived with no deps should not re-evaluate on subsequent reads'
);
const isClean = (d!.f & CLEAN) !== 0;
assert.ok(isClean, 'derived with no deps should be CLEAN');
};
});
// reproduction of https://github.com/sveltejs/svelte/issues/17352
test('nested deriveds with parent-child dependencies stay reactive', () => {
const log: any[] = [];
return () => {
const expanded_ids = new SvelteSet<string>();
const nodes = proxy([
{
id: 'folder',
children: [{ id: 'file' }]
},
{ id: 'other' }
]);
let items_derived: Derived<any[]>;
let visible_items_derived: Derived<any[]>;
const destroy = effect_root(() => {
items_derived = derived(function create_items(
list: any[] = nodes,
parent?: any,
result: any[] = []
) {
for (const node of list) {
const expanded_d = derived(() => expanded_ids.has(node.id));
const visible_d = derived(() =>
parent === undefined ? true : $.get(parent.expanded_d) && $.get(parent.visible_d)
);
const item = {
node,
expanded_d,
visible_d,
get expanded() {
return $.get(expanded_d);
},
get visible() {
return $.get(visible_d);
}
};
result.push(item);
if (node.children) {
create_items(node.children, item, result);
}
}
return result;
});
visible_items_derived = derived(() => $.get(items_derived).filter((item) => item.visible));
render_effect(() => {
log.push($.get(visible_items_derived).length);
});
});
flushSync();
assert.deepEqual(log, [2], 'initial: folder and other visible');
expanded_ids.add('folder');
flushSync();
assert.deepEqual(log, [2, 3], 'after expand: folder, file, other visible');
expanded_ids.delete('folder');
flushSync();
assert.deepEqual(log, [2, 3, 2], 'after collapse');
nodes.splice(1, 1); // Remove 'other'
const snapshot = $.get(visible_items_derived!);
assert.equal(snapshot.length, 1, 'after delete: only folder visible');
flushSync();
assert.deepEqual(log, [2, 3, 2, 1], 'effect ran after delete');
expanded_ids.add('folder');
flushSync();
assert.deepEqual(
log,
[2, 3, 2, 1, 2],
'after expand post-delete: folder and file should be visible'
);
destroy();
};
});
});

Loading…
Cancel
Save