fix: don't rebase just-created batches#18117
Conversation
It's possible to rebase just-created batches. Case A: - batch A runs effects - one of these effects writes to a source. This creates a new batch B - an effect _after_ that (still part of "flush effects of batch A") executes a derived. This creates an entry in the `current` Map in batch B - batch A commits after processing batch B (`next_batch` etc logic), batch B is pending. Due to derived being part of batchB.current batch A can wrongfully think these are connected and try to rerun/add effects etc on batch B Case B: - like case A but with an additional await inside a pending snippet Case C: - batch A with source a and b, it flushes effects - one of these effects schedules batch B with b and c scheduling an async effect - batch B is deferred - batch A commits. Due to the a/b/c partial overlap it will needlessly rerun the just scheduled async effect All these cases are wrong. We fix it like this: 1. we call `this.#commit()` _before_ running the new batches, which may stick around due to having pending work, and we don't want to rebase these. This fixes case A and C 2. we capture derived values in `previous_batch` if it exists, because it means we're currently flushing effects, and derived writes belong to that batch and not a new one that might have been scheduled already. This fixes case B Discovered this while working on #18097
🦋 Changeset detectedLatest commit: 242ea7d 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 |
|
We shouldn't continue executing async work where we know the surrounding branch is destroyed already, it can leave to noisy "derived inter" warnings or even runtime errors ("cannot stringify symbol" when running a template effect with an uninitialized source). Neither should we warn about waterfalls on an already-destroyed async effect.
Fixes #18097 (though strictly speaking that particular instance is also fixed by #18117 which fixes the underlying cause for the reruns; this one is necessary in itself though, as shown by the new test)
| // currently in the process of flushing effects. These updates to deriveds belong | ||
| // to the previous batch, not the new one (which can already exist if an earlier | ||
| // effect wrote to a source). | ||
| (previous_batch ?? current_batch).capture(derived, value, true); |
There was a problem hiding this comment.
is this always true? what would it mean if you had an effect like this?
$effect(() => {
x += 1;
console.log(derived_that_depends_on_x);
});Presumably we would capture the new value of derived_that_depends_on_x in previous_batch rather than current_batch, and I'm having a hard time wrapping my head round whether that's a problem or perfectly fine
There was a problem hiding this comment.
ooh yeah that is true. Huh. We could add a set to mark_reactions to determine "no these are part of the current batch instead". But it could be pretty expensive. Mhm.
There was a problem hiding this comment.
I indeed found a test case where it fails with this change - a derived could show the pending value on unrelated changes because it's not part of the batch_values calculation. The fix is to add it to both the current and previous batch - that's fine because the commit logic will filter out values that are the same.
This is a regression from #18117 - we moved `this.#commit()` higher up but that means that `current_batch` could be nulled out / overridden through `batch.activate/deactivate` / blocker runs inside `#commit()`. Therefore restore the previous value afterwards. No changest because #18117 is not released yet. Fixes the other part of the failing SvelteKit `query.live` test. --------- Co-authored-by: Rich Harris <rich.harris@vercel.com>
We shouldn't continue executing async work where we know the surrounding
branch is destroyed already, it can leave to noisy "derived inter"
warnings or even runtime errors ("cannot stringify symbol" when running
a template effect with an uninitialized source). Neither should we warn
about waterfalls on an already-destroyed async effect.
Fixes #18097 (though strictly speaking that particular instance is also
fixed by #18117 which fixes the underlying cause for the reruns; this
one is necessary in itself though, as shown by the new test)
---------
Co-authored-by: Rich Harris <rich.harris@vercel.com>
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## svelte@5.55.6 ### Patch Changes - fix: leave stale promises to wait for a later resolution, instead of rejecting ([#18180](#18180)) - fix: keep dependencies of `$state.eager/pending` ([#18218](#18218)) - fix: reapply context after transforming error during SSR ([#18099](#18099)) - fix: don't rebase just-created batches ([#18117](#18117)) - chore: allow `null` for `pending` in typings ([#18201](#18201)) - fix: flush eager effects in production ([#18107](#18107)) - fix: rethrow error of failed iterable after calling `return()` ([#18169](#18169)) - fix: account for proxified instance when updating `bind:this` ([#18147](#18147)) - fix: ensure scheduled batch is flushed if not obsolete ([#18131](#18131)) - fix: resolve stale deriveds with latest value ([#18167](#18167)) - chore: remove unnecessary `increment_pending` calls ([#18183](#18183)) - fix: correctly compile component member expressions for SSR ([#18192](#18192)) - fix: reset `source.updated` stack traces after `flush` ([#18196](#18196)) - fix: replacing async 'blocking' strategy with 'merging' ([#18205](#18205)) - fix: allow `@debug` tags to reference awaited variables ([#18138](#18138)) - fix: re-run fallback props if dependencies update ([#18146](#18146)) - fix: abort running obsolete async branches ([#18118](#18118)) - fix: ignore comments when reading CSS values ([#18153](#18153)) - fix: wrap `Promise.all` in `save` during SSR ([#18178](#18178)) - fix: ignore false-positive errors of `$inspect` dependencies ([#18106](#18106)) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
It's possible to rebase just-created batches.
Case A:
currentMap in batch Bnext_batchetc logic), batch B is pending. Due to derived being part of batchB.current batch A can wrongfully think these are connected and try to rerun/add effects etc on batch BCase B:
Case C:
All these cases are wrong. We fix it like this:
this.#commit()before running the new batches, which may stick around due to having pending work, and we don't want to rebase these. This fixes case A and Cprevious_batchif it exists, because it means we're currently flushing effects, and derived writes belong to that batch and not a new one that might have been scheduled already. This fixes case BDiscovered this while working on #18097