fix: clear batch between runs #17423
Conversation
🦋 Changeset detectedLatest commit: a88dded 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 |
|
|
|
||
| for (const e of this.#dirty_effects) { | ||
| batch.#dirty_effects.add(e); | ||
| } |
There was a problem hiding this comment.
I don't think this part is right. An effect would rerun multiple times as a result if multiple batches exist and resolve one after the other.
Also needs a comment because I at first thought this is all effects, not just leftovers
There was a problem hiding this comment.
Hmm, hadn't considered that it might cause effects to run multiple times. In most cases this would be harmless, but definitely suboptimal.
The correct thing, I think, would be to associate the leftover effects with the boundary rather than the batch, such that we can reschedule them when the boundary resolves. That would be a slightly meatier change so maybe I'll have a go at it in a different branch; if we want to merge this in the meantime in the interests of forward motion then I'm not too worried about the multiple runs thing, but understand if you'd prefer to hold off until we've explored that. I left a comment explaining what's going on here, with a TODO for the boundary effect thing
(about to get on a plane where I don't expect to have a) wifi or b) room to open my laptop, so may not get to this for a bit)
There was a problem hiding this comment.
In the interest of ITG I'm fine with the interim solution
| // TODO it would be better if the effects were associated with the | ||
| // boundary instead, and rescheduled when the boundary resolves |
There was a problem hiding this comment.
| // TODO it would be better if the effects were associated with the | |
| // boundary instead, and rescheduled when the boundary resolves | |
| // TODO this is buggy when there are multiple pending batches and they resolve | |
| // one after the other, leading to effects being executed more often than they should. | |
| // It would be better if the effects were associated with the | |
| // boundary instead, and rescheduled when the boundary resolves |
|
merged #17424, closing |
Found while investigating #17409 — we don't unset
current_batchafter each iteration of$.run(...), which means that in certain cases changing state has no effect (as no new batch is created, and thus noflushoccurs).Additionally, it's possible for a batch to be committed (and therefore removed) while it still has dirty effects that were deferred (though only if the effect lives inside a boundary that is pending because of a different batch, which is something of an edge case). I think the fix here makes sense — when rebasing, add leftover effects to the remaining batch(es).
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