Skip to content
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/clever-months-clap.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'svelte': patch
---

fix: only abort effect flushing if it causes an existing effect to be scheduled
2 changes: 2 additions & 0 deletions packages/svelte/src/internal/client/dom/operations.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { get_descriptor, is_extensible } from '../../shared/utils.js';
import { active_effect } from '../runtime.js';
import { async_mode_flag } from '../../flags/index.js';
import { TEXT_NODE, EFFECT_RAN } from '#client/constants';
import { eager_block_effects } from '../reactivity/batch.js';

// export these for reference in the compiled code, making global name deduplication unnecessary
/** @type {Window} */
Expand Down Expand Up @@ -214,6 +215,7 @@ export function clear_text_content(node) {
*/
export function should_defer_append() {
if (!async_mode_flag) return false;
if (eager_block_effects !== null) return false;

var flags = /** @type {Effect} */ (active_effect).f;
return (flags & EFFECT_RAN) !== 0;
Expand Down
36 changes: 19 additions & 17 deletions packages/svelte/src/internal/client/reactivity/batch.js
Original file line number Diff line number Diff line change
Expand Up @@ -292,12 +292,12 @@ export class Batch {
if (!skip && effect.fn !== null) {
if (is_branch) {
effect.f ^= CLEAN;
} else if ((flags & EFFECT) !== 0) {
this.#effects.push(effect);
} else if (async_mode_flag && (flags & RENDER_EFFECT) !== 0) {
this.#render_effects.push(effect);
} else if ((flags & CLEAN) === 0) {
if ((flags & EFFECT) !== 0) {
this.#effects.push(effect);
} else if (async_mode_flag && (flags & RENDER_EFFECT) !== 0) {
this.#render_effects.push(effect);
} else if ((flags & ASYNC) !== 0) {
if ((flags & ASYNC) !== 0) {
var effects = effect.b?.pending ? this.#boundary_async_effects : this.#async_effects;
effects.push(effect);
} else if (is_dirty(effect)) {
Expand Down Expand Up @@ -584,6 +584,9 @@ function infinite_loop_guard() {
}
}

/** @type {Effect[] | null} */
export let eager_block_effects = null;

/**
* @param {Array<Effect>} effects
* @returns {void}
Expand All @@ -598,7 +601,7 @@ function flush_queued_effects(effects) {
var effect = effects[i++];

if ((effect.f & (DESTROYED | INERT)) === 0 && is_dirty(effect)) {
var n = current_batch ? current_batch.current.size : 0;
eager_block_effects = [];

update_effect(effect);

Expand All @@ -619,21 +622,20 @@ function flush_queued_effects(effects) {
}
}

// if state is written in a user effect, abort and re-schedule, lest we run
// effects that should be removed as a result of the state change
if (
current_batch !== null &&
current_batch.current.size > n &&
(effect.f & USER_EFFECT) !== 0
) {
break;
if (eager_block_effects.length > 0) {
// TODO this feels incorrect! it gets the tests passing
old_values.clear();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the correct place since it's equivalent timing to before: previously you would bail the loop and start a new flush. That means you ended up at the end of the flush_effects loop where we also call clear(), and then did another flush.
I have some nagging doubts if the logic in general for this is sound but we can tackle that later.


for (const e of eager_block_effects) {
update_effect(e);
}

eager_block_effects = [];
}
}
}

while (i < length) {
schedule_effect(effects[i++]);
}
eager_block_effects = null;
}

/**
Expand Down
8 changes: 7 additions & 1 deletion packages/svelte/src/internal/client/reactivity/sources.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ import * as e from '../errors.js';
import { legacy_mode_flag, tracing_mode_flag } from '../../flags/index.js';
import { get_stack, tag_proxy } from '../dev/tracing.js';
import { component_context, is_runes } from '../context.js';
import { Batch, schedule_effect } from './batch.js';
import { Batch, eager_block_effects, schedule_effect } from './batch.js';
import { proxy } from '../proxy.js';
import { execute_derived } from './deriveds.js';

Expand Down Expand Up @@ -334,6 +334,12 @@ function mark_reactions(signal, status) {
if ((flags & DERIVED) !== 0) {
mark_reactions(/** @type {Derived} */ (reaction), MAYBE_DIRTY);
} else if (not_dirty) {
if ((flags & BLOCK_EFFECT) !== 0) {
if (eager_block_effects !== null) {
eager_block_effects.push(/** @type {Effect} */ (reaction));
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My guess for why this is necessary is that in the next batch that was scheduled due to this we need it for traversal of the graph.
That said ideally we could avoid or abort the next batch flush completely if we determine that it didn't schedule anything else. A possible optimization for some other time.


schedule_effect(/** @type {Effect} */ (reaction));
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<script>
let { children } = $props();

let inited = $state(false);

$effect(() => {
inited = true;
});
</script>

{#if inited}
<span>{@render children()}</span>
{/if}
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import { flushSync } from 'svelte';
import { test } from '../../test';

export default test({
async test({ assert, target }) {
const [button] = target.querySelectorAll('button');

assert.doesNotThrow(() => {
flushSync(() => button.click());
});
}
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
<script>
import Child from './Child.svelte';

let show = $state(false);
</script>

<button onclick={() => show = !show}>
toggle
</button>

{#if show}
{#each { length: 1234 } as i}
<Child>{i}</Child>
{/each}
{/if}
Loading