-
-
Notifications
You must be signed in to change notification settings - Fork 4.9k
fix: skip derived re-evaluation inside destroyed branch effects #17862
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
mtsgrd
wants to merge
7
commits into
sveltejs:main
Choose a base branch
from
mtsgrd:gitbutler-bug-2
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
de3757a
fix: skip derived re-evaluation inside destroyed branch effects
mtsgrd ba44bdd
fix: guard derived re-evaluation inside INERT (outroing) branches
mtsgrd afd35cf
fix: capture computed bind:this keys to prevent stale reads on teardown
mtsgrd 10a00b9
Merge branch 'main' into gitbutler-bug-2
Rich-Harris 8e0cfc8
typecheck
Rich-Harris 1bb8442
remove the bind:this stuff
Rich-Harris 489cc44
merge main
Rich-Harris File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| 'svelte': patch | ||
| --- | ||
|
|
||
| fix: skip derived re-evaluation inside destroyed branch effects |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -12,7 +12,9 @@ import { | |||||||||||||||||||||
| WAS_MARKED, | ||||||||||||||||||||||
| DESTROYED, | ||||||||||||||||||||||
| CLEAN, | ||||||||||||||||||||||
| REACTION_RAN | ||||||||||||||||||||||
| INERT, | ||||||||||||||||||||||
| REACTION_RAN, | ||||||||||||||||||||||
| BRANCH_EFFECT | ||||||||||||||||||||||
| } from '#client/constants'; | ||||||||||||||||||||||
| import { | ||||||||||||||||||||||
| active_reaction, | ||||||||||||||||||||||
|
|
@@ -40,7 +42,7 @@ import { get_error } from '../../shared/dev.js'; | |||||||||||||||||||||
| import { async_mode_flag, tracing_mode_flag } from '../../flags/index.js'; | ||||||||||||||||||||||
| import { component_context } from '../context.js'; | ||||||||||||||||||||||
| import { UNINITIALIZED } from '../../../constants.js'; | ||||||||||||||||||||||
| import { batch_values, current_batch } from './batch.js'; | ||||||||||||||||||||||
| import { batch_values, collected_effects, current_batch } from './batch.js'; | ||||||||||||||||||||||
| import { increment_pending, unset_context } from './async.js'; | ||||||||||||||||||||||
| import { deferred, includes, noop } from '../../shared/utils.js'; | ||||||||||||||||||||||
| import { set_signal_status, update_derived_status } from './status.js'; | ||||||||||||||||||||||
|
|
@@ -315,9 +317,7 @@ function get_derived_parent_effect(derived) { | |||||||||||||||||||||
| var parent = derived.parent; | ||||||||||||||||||||||
| while (parent !== null) { | ||||||||||||||||||||||
| if ((parent.f & DERIVED) === 0) { | ||||||||||||||||||||||
| // The original parent effect might've been destroyed but the derived | ||||||||||||||||||||||
| // is used elsewhere now - do not return the destroyed effect in that case | ||||||||||||||||||||||
| return (parent.f & DESTROYED) === 0 ? /** @type {Effect} */ (parent) : null; | ||||||||||||||||||||||
| return /** @type {Effect} */ (parent); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| parent = parent.parent; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
@@ -330,10 +330,24 @@ function get_derived_parent_effect(derived) { | |||||||||||||||||||||
| * @returns {T} | ||||||||||||||||||||||
| */ | ||||||||||||||||||||||
| export function execute_derived(derived) { | ||||||||||||||||||||||
| var raw_parent = get_derived_parent_effect(derived); | ||||||||||||||||||||||
| var parent_effect = raw_parent !== null && (raw_parent.f & DESTROYED) !== 0 ? null : raw_parent; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // don't update deriveds inside a destroyed branch (e.g. {#if} or {#each}) — | ||||||||||||||||||||||
| // the branch scope is invalid and evaluating could trigger side effects | ||||||||||||||||||||||
| // with stale values. | ||||||||||||||||||||||
| if ( | ||||||||||||||||||||||
| !is_destroying_effect && | ||||||||||||||||||||||
| raw_parent !== null && | ||||||||||||||||||||||
| (raw_parent.f & (DESTROYED | BRANCH_EFFECT)) === (DESTROYED | BRANCH_EFFECT) | ||||||||||||||||||||||
| ) { | ||||||||||||||||||||||
| return derived.v; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
Comment on lines
+336
to
+346
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. appears we never reach this code, because of the guard in
Suggested change
|
||||||||||||||||||||||
| var value; | ||||||||||||||||||||||
| var prev_active_effect = active_effect; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| set_active_effect(get_derived_parent_effect(derived)); | ||||||||||||||||||||||
| set_active_effect(parent_effect); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| if (DEV) { | ||||||||||||||||||||||
| let prev_eager_effects = eager_effects; | ||||||||||||||||||||||
|
|
@@ -371,6 +385,32 @@ export function execute_derived(derived) { | |||||||||||||||||||||
| * @returns {void} | ||||||||||||||||||||||
| */ | ||||||||||||||||||||||
| export function update_derived(derived) { | ||||||||||||||||||||||
| // Don't re-evaluate deriveds inside INERT (outroing) branches when the | ||||||||||||||||||||||
| // read originates from outside the branch. Re-evaluating would use stale | ||||||||||||||||||||||
| // dependency values (e.g. a prop that became `undefined` when the branch | ||||||||||||||||||||||
| // condition changed), violating the `{#if}` contract. | ||||||||||||||||||||||
| // | ||||||||||||||||||||||
| // In non-async mode, INERT branches are never walked by the scheduler, | ||||||||||||||||||||||
| // so any read is necessarily external — block unconditionally. | ||||||||||||||||||||||
| // | ||||||||||||||||||||||
| // In async mode, INERT branches ARE walked (to keep transitions alive), | ||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is outdated now, might simplify some stuff |
||||||||||||||||||||||
| // so we only block reads during effect flushing (collected_effects === null | ||||||||||||||||||||||
| // and active_effect === null), which indicates the reader is an external | ||||||||||||||||||||||
| // effect, not the branch's own traversal. | ||||||||||||||||||||||
| if (!is_destroying_effect) { | ||||||||||||||||||||||
| var dominated_by_inert = async_mode_flag | ||||||||||||||||||||||
| ? collected_effects === null && active_effect === null | ||||||||||||||||||||||
| : true; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| if (dominated_by_inert) { | ||||||||||||||||||||||
| var parent = get_derived_parent_effect(derived); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| if (parent !== null && (parent.f & INERT) !== 0 && (parent.f & DESTROYED) === 0) { | ||||||||||||||||||||||
| return; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| var old_value = derived.v; | ||||||||||||||||||||||
| var value = execute_derived(derived); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
|
||||||||||||||||||||||
9 changes: 9 additions & 0 deletions
9
...es/svelte/tests/runtime-runes/samples/if-block-derived-inert-external-reader/Inner.svelte
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| <script> | ||
| let { data } = $props(); | ||
|
|
||
| const processed = $derived(data.toUpperCase()); | ||
|
|
||
| export function getProcessed() { | ||
| return processed; | ||
| } | ||
| </script> |
36 changes: 36 additions & 0 deletions
36
...ages/svelte/tests/runtime-runes/samples/if-block-derived-inert-external-reader/_config.js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| import { flushSync } from 'svelte'; | ||
| import { test } from '../../test'; | ||
|
|
||
| // Covers the INERT (outroing) counterpart to if-block-const-destroyed-external-reader. | ||
| // An external $derived reads a child component's $derived via bind:this, keeping it | ||
| // connected in the reactive graph while the branch is outroing. Without a guard, | ||
| // the inner derived re-evaluates with stale values mid-transition and crashes. | ||
| // The fix returns the cached value and keeps the derived dirty so it re-evaluates | ||
| // correctly if the branch reverses (INERT cleared) rather than being stuck with | ||
| // a stale clean value. | ||
| export default test({ | ||
| ssrHtml: '<div></div><button>clear</button><p></p>', | ||
| html: '<div></div><button>clear</button><p>HELLO</p>', | ||
|
|
||
| async test({ assert, raf, target }) { | ||
| const [button] = target.querySelectorAll('button'); | ||
|
|
||
| // Clearing value starts the out-transition (branch becomes INERT). | ||
| // Without the guard this crashes with a TypeError in async mode. | ||
| flushSync(() => button.click()); | ||
|
|
||
| assert.htmlEqual( | ||
| target.innerHTML, | ||
| '<div style="opacity: 0;"></div><button>clear</button><p>HELLO</p>' | ||
| ); | ||
|
|
||
| // Complete the transition — branch is now destroyed and div is removed. | ||
| raf.tick(100); | ||
|
|
||
| // Flush the bind:this teardown microtask and resulting effect updates. | ||
| await Promise.resolve(); | ||
| flushSync(); | ||
|
|
||
| assert.htmlEqual(target.innerHTML, '<button>clear</button><p></p>'); | ||
| } | ||
| }); |
21 changes: 21 additions & 0 deletions
21
...ges/svelte/tests/runtime-runes/samples/if-block-derived-inert-external-reader/main.svelte
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| <script> | ||
| import { fade } from 'svelte/transition'; | ||
| import Inner from './Inner.svelte'; | ||
|
|
||
| let value = $state('hello'); | ||
|
|
||
| let innerComp = $state(); | ||
|
|
||
| // Reads Inner's derived value from outside the {#if} block, keeping it | ||
| // connected in the reactive graph even when the branch is outroing. | ||
| const externalView = $derived(innerComp?.getProcessed() ?? ''); | ||
| </script> | ||
|
|
||
| {#if value} | ||
| <div out:fade={{ duration: 100 }}> | ||
| <Inner data={value} bind:this={innerComp} /> | ||
| </div> | ||
| {/if} | ||
|
|
||
| <button onclick={() => (value = undefined)}>clear</button> | ||
| <p>{externalView}</p> |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.