diff --git a/.changeset/quiet-deriveds-unwind.md b/.changeset/quiet-deriveds-unwind.md new file mode 100644 index 0000000000..8036bdb781 --- /dev/null +++ b/.changeset/quiet-deriveds-unwind.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +fix: keep the dependencies of a reaction that throws, so deriveds it read are neither leaked nor stuck in their error diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js index e0914b8f70..a881403f58 100644 --- a/packages/svelte/src/internal/client/runtime.js +++ b/packages/svelte/src/internal/client/runtime.js @@ -258,37 +258,7 @@ export function update_reaction(reaction) { var fn = /** @type {Function} */ (reaction.fn); var result = fn(); reaction.f |= REACTION_RAN; - var deps = reaction.deps; - - // Don't remove reactions during fork; - // they must remain for when fork is discarded - var is_fork = current_batch?.is_fork; - - if (new_deps !== null) { - var i; - - if (!is_fork) { - remove_reactions(reaction, skipped_deps); - } - - if (deps !== null && skipped_deps > 0) { - deps.length = skipped_deps + new_deps.length; - for (i = 0; i < new_deps.length; i++) { - deps[skipped_deps + i] = new_deps[i]; - } - } else { - reaction.deps = deps = new_deps; - } - - if (effect_tracking() && (reaction.f & CONNECTED) !== 0) { - for (i = skipped_deps; i < deps.length; i++) { - (deps[i].reactions ??= []).push(reaction); - } - } - } else if (!is_fork && deps !== null && skipped_deps < deps.length) { - remove_reactions(reaction, skipped_deps); - deps.length = skipped_deps; - } + var deps = update_dependencies(reaction); // If we're inside an effect and we have untracked writes, then we need to // ensure that if any of those untracked writes result in re-invalidation @@ -300,7 +270,7 @@ export function update_reaction(reaction) { deps !== null && (reaction.f & (DERIVED | MAYBE_DIRTY | DIRTY)) === 0 ) { - for (i = 0; i < /** @type {Source[]} */ (untracked_writes).length; i++) { + for (var i = 0; i < /** @type {Source[]} */ (untracked_writes).length; i++) { schedule_possible_effect_self_invalidation( untracked_writes[i], /** @type {Effect} */ (reaction) @@ -344,6 +314,9 @@ export function update_reaction(reaction) { return result; } catch (error) { + // still commit the deps read before the throw, otherwise deriveds connected by this run keep no reader and the reaction never re-runs when they change + update_dependencies(reaction); + return handle_error(error); } finally { reaction.f ^= REACTION_IS_UPDATING; @@ -358,6 +331,45 @@ export function update_reaction(reaction) { } } +/** + * @param {Reaction} reaction + */ +function update_dependencies(reaction) { + var deps = reaction.deps; + + // Don't remove reactions during fork; + // they must remain for when fork is discarded + var is_fork = current_batch?.is_fork; + + if (new_deps !== null) { + var i; + + if (!is_fork) { + remove_reactions(reaction, skipped_deps); + } + + if (deps !== null && skipped_deps > 0) { + deps.length = skipped_deps + new_deps.length; + for (i = 0; i < new_deps.length; i++) { + deps[skipped_deps + i] = new_deps[i]; + } + } else { + reaction.deps = deps = new_deps; + } + + if (effect_tracking() && (reaction.f & CONNECTED) !== 0) { + for (i = skipped_deps; i < deps.length; i++) { + (deps[i].reactions ??= []).push(reaction); + } + } + } else if (!is_fork && deps !== null && skipped_deps < deps.length) { + remove_reactions(reaction, skipped_deps); + deps.length = skipped_deps; + } + + return deps; +} + /** * @template V * @param {Reaction} signal diff --git a/packages/svelte/tests/signals/test.ts b/packages/svelte/tests/signals/test.ts index 07f79bd395..c1ebc3ec75 100644 --- a/packages/svelte/tests/signals/test.ts +++ b/packages/svelte/tests/signals/test.ts @@ -1521,4 +1521,74 @@ describe('signals', () => { assert.equal(s.reactions, null); }; }); + + // https://github.com/sveltejs/svelte/issues/18414 + test('a reaction that throws after first-reading a fresh derived does not leak it', () => { + const src = state(0); + const pane = derived(() => $.get(src)); + const base = derived(() => $.get(pane) + ':base'); + const extra = derived(() => $.get(pane) + ':extra'); + const flag = state(false); + + const destroy = effect_root(() => { + render_effect(() => { + if ($.get(flag)) { + $.get(extra); + throw new Error('render boom'); + } else { + $.get(base); + } + }); + }); + + return () => { + try { + flushSync(() => set(flag, true)); + } catch {} + + destroy(); + + assert.equal(src.reactions, null); + }; + }); + + test('a derived that throws on its first run re-runs when its dependencies change', () => { + const s = state(0); + const fn = () => { + if ($.get(s) === 0) throw new Error('boom'); + return $.get(s); + }; + const owned = derived(fn); + const previous_effect = $.active_effect; + $.set_active_effect(null); + const unowned = derived(fn); + $.set_active_effect(previous_effect); + const log: any[] = []; + + const destroy = effect_root(() => { + render_effect(() => { + for (const d of [owned, unowned]) { + try { + log.push($.get(d)); + } catch { + log.push('error'); + } + } + }); + }); + + return () => { + assert.notEqual(owned.parent, null); + assert.equal(unowned.parent, null); + + flushSync(); + assert.deepEqual(log, ['error', 'error']); + + flushSync(() => set(s, 1)); + assert.deepEqual(log, ['error', 'error', 1, 1]); + + destroy(); + assert.equal(s.reactions, null); + }; + }); });