fix: ensure deferred effects can be rescheduled later on
#17147
Closing issue
Describe the bug
With hover preloading on and async enabled, it seems that conditional bind:this can somehow break reactivity for the application.
Minimal repro below. The select box uses the bind:this for absolutely nothing in the repro, but it still breaks. Remove the bind:this, disable preloading, or disable async and things behave as expected.
Reproduction
Logs
System Info
System:
OS: Windows 10 10.0.19045
CPU: (32) x64 AMD Ryzen 9 5950X 16-Core Processor
Memory: 31.45 GB / 63.89 GB
Binaries:
Node: 22.12.0 - C:\nvm4w\nodejs\node.EXE
npm: 10.9.0 - C:\nvm4w\nodejs\npm.CMD
Browsers:
Chrome: 141.0.7390.123
Edge: Chromium (140.0.3485.54)
Firefox: 144.0.2 - C:\Program Files\Mozilla Firefox\firefox.exe
Firefox Developer Edition: 137.0 - C:\Program Files\Firefox Developer Edition\firefox.exe
Internet Explorer: 11.0.19041.5794Severity
annoyance
Pull request
When deferring effects we didn't unmark the deriveds that lead to those effects. This means that they might not be reached in subsequent runs of mark_reactions.
Fixes #17118 (comment)
Before submitting the PR, please make sure you do the following
- It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
- Prefix your PR title with
feat:,fix:,chore:, ordocs:. - This message body should clearly illustrate what problems it solves.
- Ideally, include a test that fails without this PR but passes with it.
- If this PR changes code within
packages/svelte/src, add a changeset (npx changeset).
Tests and linting
- Run the tests with
pnpm testand lint the project withpnpm lint
Info
![changeset-bot[bot]](https://avatars.githubusercontent.com/in/28616?v=4)
🦋 Changeset detected
Latest commit: 8f7e8b9
The changes in this PR will be included in the next version bump.
This PR includes changesets to release 1 package
| Name | Type |
|---|---|
| svelte | Patch |
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
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?v=4)
pnpm add https://pkg.pr.new/svelte@17147
Btw to fully solve this in SvelteKit I think we also need to discard the last fork when a new one come because I think right now we don't do it.
Why though? It should be possible to have parallel forks. They could be independent and it would be very confusing to have a fork be discarded when another part of the app you may not have control over starts a fork
Mmm I'm specifically talking about preloading tho... shouldn't we discard older fork when you move your mouse on another link?
Looking at SvelteKit's code it should alread do that (discard_load_cache)
It only do that inside goto, invalidateAll or if it errors...we also do that in preload before creating the new load_cache (this also causes await reactivity loss warnings). I might be wrong tho.
Moving your mouse on another link should call preload and therefore discard the old cache, no?
Yeah but right now preload doesn't discard the cache unless it's an error...that's the issue.
Lul just realized a missed a "should" before the last sentence 😅
@paoloricciuti I think Rich already implemented it sveltejs/kit#14865 it is just not released yet. But TBH I don't really understand why we need that. Wouldn't it be enough to discard all other forks when the user finally clicks a link.
When deferring effects we didn't unmark the deriveds that lead to those effects. This means that they might not be reached in subsequent runs of `mark_reactions`. Fixes https://github.com/sveltejs/svelte/issues/17118#issuecomment-3521488865
Pro tip: You can prefix GitHub URLs of issues, PRs or discussions with svcl.dev/ to view them on this page! Also try it on a GitHub release ;)