Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork4.7k
fix: run blocks eagerly during flush#16631
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
🦋 Changeset detectedLatest commit:5d6b081 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means?Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
| break; | ||
| if(eager_block_effects.length>0){ | ||
| // TODO this feels incorrect! it gets the tests passing | ||
| old_values.clear(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is the correct place since it's equivalent timing to before: previously you would bail the loop and start a new flush. That means you ended up at the end of theflush_effects loop where we also callclear(), and then did another flush.
I have some nagging doubts if the logic in general for this is sound but we can tackle that later.
| if(eager_block_effects!==null){ | ||
| eager_block_effects.push(/**@type {Effect} */(reaction)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
My guess for why this is necessary is that in the next batch that was scheduled due to this we need it for traversal of the graph.
That said ideally we could avoid or abort the next batch flush completely if we determine that it didn't schedule anything else. A possible optimization for some other time.
Uh oh!
There was an error while loading.Please reload this page.
2b85d2a intomainUh oh!
There was an error while loading.Please reload this page.
Prajwalg19 commentedAug 17, 2025
Fixes#16850,fixes#16775,fixes#16795,fixes#16982#16631 introduced a bug that results in the effects within guards being evaluated before the guards themselves. This fix makes sure to iterate the block effects in the correct order (top down)---------Co-authored-by: Simon H <5968653+dummdidumm@users.noreply.github.com>Co-authored-by: Simon Holthausen <simon.holthausen@vercel.com>
Another stab at#16548. The idea is that if a block effect is dirtied by a state change in another effect, it jumps to the head of the queue rather than waiting for the next root-to-tip traversal. That way, if it ends up destroying its children, they get destroyed immediately, rather than running even though they'reabout to be destroyed (which can result in errors —#16072).
Because we're not causing extra traversals, this (as far as I've been able to determine) fully removes the performance overhead introduced by#16280, without regressing on correctness. It also fixes the false positive infinite loop detection.
The one part I really don't love is the
old_values.clear()— it gets the tests passing (one fails without it) but feels wrong. I stole it from#16612 — maybe@dummdidumm can explain if/why/how it's okay.It also seems weird that we need to schedule the block effects inaddition to running them eagerly, but if we don't then one test fails. Will investigate when my brain isn't running on fumes.
Before submitting the PR, please make sure you do the following
feat:,fix:,chore:, ordocs:.packages/svelte/src, add a changeset (npx changeset).Tests and linting
pnpm testand lint the project withpnpm lint