fix: preserve dirty status of deferred effects #16487
Conversation
🦋 Changeset detectedLatest commit: 51f24a3 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 |
|
mini-cleanup post #16487 - we don't need to do the work of scheduling an effect that's already dirty which means it already scheduled its root effect to run
mini-cleanup post #16487 - we don't need to do the work of scheduling an effect that's already dirty which means it already scheduled its root effect to run
|
I'm going to post this here because, in decades of trying to break javascript and write the worst stuff ever, I have never, ever seen this error... ...and I suspect it's related to this issue/set of commits. I'm not saying Svelte is at all to blame. I'm just trying to debug what is probably really bad state management in our (my company's) code. It looks like stuff is being recursively spread into a function, until such time as the limit is reached. I can't share the code that causes this, but if you like I can try to come up with a minimal repro. LMK, and thanks for all your work! |
|
Sounds like a |
|
Ok, I'll check our code first for that. Thanks. |
If you have a value X that causes a pending batch, and while that batch is pending cause creation of another batch which creates a new branch and therefore new effects which also read that value X, those template effects are not correctly rescheduled once the pending batch resolves because the pending batch does not know about them. As a result, the shown values of X get out of sync (once shows the old, once the new value). This partially fixes that by telling the pending batch about the newly created effects. Partially fixes #17099, but not completely because effects that are only indirectly relying on the source that change update, but those that directly rely on the source do not. In this example that means if you do `{x}` in the template it does not work, because it directly invokes the source x, but if I do e.g. `{JSON.stringify(x)}` it does work because the intermediate derived is marked as outdated and therefore reruns, reinvoking the source getter in the process. I assume it's because `MAYBE_DIRTY` for the effect isn't enough, since the version check logic determines that the effect's dependencies aren't newer. I believe we can only fix that by changing the strategy and going back to rerunning `mark_reactions` on changed sources in a batch similar to how it was before #16487 but without running into the infinite loop problem, possibly by keeping track of which of these reactions are async effects that have already run.
Ironically, the part of #16482 that fixed the infinite loop bug in #16403 introduced an even worse infinite loop that made remote functions unusable. I have learned, for the 19th or so time, that it is absolutely not safe to
mark_reactionswhen triggering a batch re-run.It turns out we (which in this case is the royal 'we') were doing something rather silly: we were queueing all leaf effects when traversing the tree, not just the
DIRTY/MAYBE_DIRTYones. In the common case that's fine, because they get flushed immediately and they get filtered withis_dirty, but if the effects are deferred (because async work is happening) then on the next run they are all markedDIRTYand re-executed.This PR makes everything somewhat more efficient. Firstly, it only queues effects that are not
CLEAN. (We can't use anis_dirtycheck during traversal, because that can cause deriveds to be re-evaluated with inconsistent data.) Secondly, it notes whether they wereDIRTYorMAYBE_DIRTYat the time of deferral, and restores that status to prevent unnecessary re-runs ofMAYBE_DIRTYeffects.I wasn't able to whittle the remote functions infinite loop thing down to a test case, but I verified the fix locally which will have to do for now as it's 1am and I'm supposed to be demoing this stuff on YouTube tomorrow.
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