chore: avoid reschedule during branch commit (#17837)

If a dirty effect is resumed — for example `condition` becomes `false`
then `count` changes then `condition` becomes `true` again...

```svelte
{#if condition}
  <div transition:fade>
    {count}
  </div>
{/if}
```

...then the effect is rescheduled. This happens when branches are
committed (after the effect tree is traversed, before effects are
flushed).

That's undesirable, because it causes another turn of the
`flush_effects` loop. It's better if we can handle everything in a
single pass, which is what happens in this PR. The trade-off is that we
have to traverse the entire effect tree, instead of skipping inert
subtrees, which is a trade-off that I think makes sense.

The real agenda here is that I'm trying to eliminate all
`schedule_effect` calls that happen at inconvenient times, because I
have a hunch that if we do that we can return to #17805, which I'm
increasingly convinced will be important. (You might have to trust me on
this; a full explanation would look a bit charlie-day-meme.jpg. Call it
a hunch.)

### Before submitting the PR, please make sure you do the following

- [x] 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
- [x] Prefix your PR title with `feat:`, `fix:`, `chore:`, or `docs:`.
- [x] This message body should clearly illustrate what problems it
solves.
- [ ] Ideally, include a test that fails without this PR but passes with
it.
- [x] If this PR changes code within `packages/svelte/src`, add a
changeset (`npx changeset`).

### Tests and linting

- [x] Run the tests with `pnpm test` and lint the project with `pnpm
lint`

---------

Co-authored-by: Vercel <vercel[bot]@users.noreply.github.com>
pull/17842/head
Rich Harris 5 months ago committed by GitHub
parent 00dc13db14
commit 2f12b60701
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194

@ -0,0 +1,5 @@
---
'svelte': patch
---
chore: avoid rescheduling effects during branch commit

@ -1,4 +1,5 @@
/** @import { Effect, TemplateNode } from '#client' */
import { INERT } from '#client/constants';
import { Batch, current_batch } from '../../reactivity/batch.js';
import {
branch,
@ -87,7 +88,7 @@ export class BranchManager {
// effect is currently offscreen. put it in the DOM
var offscreen = this.#offscreen.get(key);
if (offscreen) {
if (offscreen && (offscreen.effect.f & INERT) === 0) {
this.#onscreen.set(key, offscreen.effect);
this.#offscreen.delete(key);
@ -124,6 +125,9 @@ export class BranchManager {
// or those that are already outroing (else the transition is aborted and the effect destroyed right away)
if (k === key || this.#outroing.has(k)) continue;
// don't destroy branches that are inside outroing blocks
if ((effect.f & INERT) !== 0) continue;
const on_destroy = () => {
const keys = Array.from(this.#batches.values());

@ -266,18 +266,26 @@ export class Batch {
var is_branch = (flags & (BRANCH_EFFECT | ROOT_EFFECT)) !== 0;
var is_skippable_branch = is_branch && (flags & CLEAN) !== 0;
var skip = is_skippable_branch || (flags & INERT) !== 0 || this.#skipped_branches.has(effect);
var inert = (flags & INERT) !== 0;
var skip = is_skippable_branch || this.#skipped_branches.has(effect);
if (!skip && effect.fn !== null) {
if (is_branch) {
effect.f ^= CLEAN;
if (!inert) effect.f ^= CLEAN;
} else if ((flags & EFFECT) !== 0) {
effects.push(effect);
} else if (async_mode_flag && (flags & (RENDER_EFFECT | MANAGED_EFFECT)) !== 0) {
} else if ((flags & (RENDER_EFFECT | MANAGED_EFFECT)) !== 0 && (async_mode_flag || inert)) {
render_effects.push(effect);
} else if (is_dirty(effect)) {
if ((flags & BLOCK_EFFECT) !== 0) this.#maybe_dirty_effects.add(effect);
update_effect(effect);
if ((flags & BLOCK_EFFECT) !== 0) {
this.#maybe_dirty_effects.add(effect);
// if this is inside an outroing block, ensure that the block
// re-runs if the outro is later aborted
if (inert) set_signal_status(effect, DIRTY);
}
}
var child = effect.first;

@ -44,6 +44,7 @@ import { Batch, collected_effects, schedule_effect } from './batch.js';
import { flatten, increment_pending } from './async.js';
import { without_reactive_context } from '../dom/elements/bindings/shared.js';
import { set_signal_status } from './status.js';
import { async_mode_flag } from '../../flags/index.js';
/**
* @param {'$effect' | '$effect.pre' | '$inspect'} rune
@ -665,13 +666,10 @@ function resume_children(effect, local) {
if ((effect.f & INERT) === 0) return;
effect.f ^= INERT;
// If a dependency of this effect changed while it was paused,
// schedule the effect to update. we don't use `is_dirty`
// here because we don't want to eagerly recompute a derived like
// `{#if foo}{foo.bar()}{/if}` if `foo` is now `undefined
if ((effect.f & CLEAN) === 0) {
set_signal_status(effect, DIRTY);
schedule_effect(effect);
// Mark branches as clean so that effects can be scheduled, but only in async mode
// (in legacy mode, effect resumption happens during traversal)
if (async_mode_flag && (effect.f & BRANCH_EFFECT) !== 0 && (effect.f & CLEAN) === 0) {
effect.f ^= CLEAN;
}
var child = effect.first;

Loading…
Cancel
Save