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: only abort effect flushing if it causes an existing effect to be scheduled#16623
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-botbot commentedAug 14, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
🦋 Changeset detectedLatest commit:87e5d00 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 |
|
Rich-Harris commentedAug 15, 2025
Inf89e355 I tried restricting things to block effects, on the basis that (I think?) the only way#16072 could reoccur is if a block containing a scheduled effect is destroyed. It causes one of the finicky ordering tests to fail, so I don't know if it's the right fix, but I'm curious as to whether it'll address#16548 (comment). Of note: while it helps in the case that you're renderingthis component many times... <script>let inited=$state(false);$effect(()=> { inited=true; });</script><p>{inited}</p> ...you do still get an erroneous infinite loop error if the <script>let inited=$state(false);$effect(()=> { inited=true; });</script>{#ifinited} <p>{inited}</p>{/if} |
dummdidumm commentedAug 15, 2025
Pretty sure you can fix the test by doing the same as I did in my latest commit to#16612 which is collecting all user/render effects of dirty branches regardless of whether they are dirty or not. |
dummdidumm commentedAug 17, 2025
closing in favor of#16631 |
Alternative to#16612 which I think is probably a bit simpler, and hopefully results in a greater performance boost.
Whereas#16612 works by flushing parts of the effect tree when an invalidation occurs, such that to-be-destroyed effects are destroyed before they can re-execute, this comes from the opposite end — we continue to abort flushing if a state change occurs in a user effect, butonly if that state change results in an existing effect being marked (the cause of#16072).
Fixes#16548.
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