fix: don't execute attachments and attribute effects eagerly (#17208)

* fix: don't execute attachments and attribute effects eagerly

attributes_effect and attachments are blocks since they need the managed "don't just destroy children effects"-behavior, but they're not block effects in the sense of "run them eagerly while traversing the effect tree or while flushing effects". Since the latter was the case until now, it meant that forks could cause visible UI updates.

This PR introduces a new flag to fix that. `BLOCK_NON_EAGER` is basically a combination of block effects (with respects to the managed behavior) and render effects (with respects to the execution timing).

Fixes sveltejs/kit#14931

* managed_effect

Author: dummdidumm

.changeset/curvy-clouds-cut.md

@@ -0,0 +1,5 @@
+---
+'svelte': patch
+---
+
+fix: don't execute attachments and attribute effects eagerly

packages/svelte/src/internal/client/constants.js

@@ -2,6 +2,15 @@
 export const DERIVED = 1 << 1;
 export const EFFECT = 1 << 2;
 export const RENDER_EFFECT = 1 << 3;
+/**
+ * An effect that does not destroy its child effects when it reruns.
+ * Runs as part of render effects, i.e. not eagerly as part of tree traversal or effect flushing.
+ */
+export const MANAGED_EFFECT = 1 << 24;
+/**
+ * An effect that does not destroy its child effects when it reruns (like MANAGED_EFFECT).
+ * Runs eagerly as part of tree traversal or effect flushing.
+ */
 export const BLOCK_EFFECT = 1 << 4;
 export const BRANCH_EFFECT = 1 << 5;
 export const ROOT_EFFECT = 1 << 6;

packages/svelte/src/internal/client/dom/elements/attachments.js

@@ -1,5 +1,5 @@
 /** @import { Effect } from '#client' */
-import { block, branch, effect, destroy_effect } from '../../reactivity/effects.js';
+import { branch, effect, destroy_effect, managed } from '../../reactivity/effects.js';
 
 // TODO in 6.0 or 7.0, when we remove legacy mode, we can simplify this by
 // getting rid of the block/branch stuff and just letting the effect rip.
@@ -16,7 +16,7 @@ export function attach(node, get_fn) {
 	/** @type {Effect | null} */
 	var e;
 
-	block(() => {
+	managed(() => {
 		if (fn !== (fn = get_fn())) {
 			if (e) {
 				destroy_effect(e);

packages/svelte/src/internal/client/dom/elements/attributes.js

@@ -20,7 +20,7 @@ import { clsx } from '../../../shared/attributes.js';
 import { set_class } from './class.js';
 import { set_style } from './style.js';
 import { ATTACHMENT_KEY, NAMESPACE_HTML, UNINITIALIZED } from '../../../../constants.js';
-import { block, branch, destroy_effect, effect } from '../../reactivity/effects.js';
+import { branch, destroy_effect, effect, managed } from '../../reactivity/effects.js';
 import { init_select, select_option } from './bindings/select.js';
 import { flatten } from '../../reactivity/async.js';
 
@@ -508,7 +508,7 @@ export function attribute_effect(
 		var is_select = element.nodeName === 'SELECT';
 		var inited = false;
 
-		block(() => {
+		managed(() => {
 			var next = fn(...values.map(get));
 			/** @type {Record<string | symbol, any>} */
 			var current = set_attributes(

packages/svelte/src/internal/client/reactivity/batch.js

@@ -17,7 +17,8 @@ import {
 	EAGER_EFFECT,
 	HEAD_EFFECT,
 	ERROR_VALUE,
-	WAS_MARKED
+	WAS_MARKED,
+	MANAGED_EFFECT
 } from '#client/constants';
 import { async_mode_flag } from '../../flags/index.js';
 import { deferred, define_property } from '../../shared/utils.js';
@@ -234,7 +235,7 @@ export class Batch {
 					effect.f ^= CLEAN;
 				} else if ((flags & EFFECT) !== 0) {
 					target.effects.push(effect);
-				} else if (async_mode_flag && (flags & RENDER_EFFECT) !== 0) {
+				} else if (async_mode_flag && (flags & (RENDER_EFFECT | MANAGED_EFFECT)) !== 0) {
 					target.render_effects.push(effect);
 				} else if (is_dirty(effect)) {
 					if ((effect.f & BLOCK_EFFECT) !== 0) target.block_effects.push(effect);
@@ -779,7 +780,7 @@ function mark_effects(value, sources, marked, checked) {
 				mark_effects(/** @type {Derived} */ (reaction), sources, marked, checked);
 			} else if (
 				(flags & (ASYNC | BLOCK_EFFECT)) !== 0 &&
-				(flags & DIRTY) === 0 && // we may have scheduled this one already
+				(flags & DIRTY) === 0 &&
 				depends_on(reaction, sources, checked)
 			) {
 				set_signal_status(reaction, DIRTY);

packages/svelte/src/internal/client/reactivity/effects.js

@@ -33,7 +33,8 @@ import {
 	STALE_REACTION,
 	USER_EFFECT,
 	ASYNC,
-	CONNECTED
+	CONNECTED,
+	MANAGED_EFFECT
 } from '#client/constants';
 import * as e from '../errors.js';
 import { DEV } from 'esm-env';
@@ -401,6 +402,18 @@ export function block(fn, flags = 0) {
 	return effect;
 }
 
+/**
+ * @param {(() => void)} fn
+ * @param {number} flags
+ */
+export function managed(fn, flags = 0) {
+	var effect = create_effect(MANAGED_EFFECT | flags, fn, true);
+	if (DEV) {
+		effect.dev_stack = dev_stack;
+	}
+	return effect;
+}
+
 /**
  * @param {(() => void)} fn
  */

packages/svelte/src/internal/client/reactivity/sources.js

@@ -363,10 +363,8 @@ function mark_reactions(signal, status) {
 				mark_reactions(derived, MAYBE_DIRTY);
 			}
 		} else if (not_dirty) {
-			if ((flags & BLOCK_EFFECT) !== 0) {
-				if (eager_block_effects !== null) {
-					eager_block_effects.add(/** @type {Effect} */ (reaction));
-				}
+			if ((flags & BLOCK_EFFECT) !== 0 && eager_block_effects !== null) {
+				eager_block_effects.add(/** @type {Effect} */ (reaction));
 			}
 
 			schedule_effect(/** @type {Effect} */ (reaction));

packages/svelte/src/internal/client/runtime.js

@@ -21,7 +21,8 @@ import {
 	REACTION_IS_UPDATING,
 	STALE_REACTION,
 	ERROR_VALUE,
-	WAS_MARKED
+	WAS_MARKED,
+	MANAGED_EFFECT
 } from './constants.js';
 import { old_values } from './reactivity/sources.js';
 import {
@@ -421,7 +422,7 @@ export function update_effect(effect) {
 	}
 
 	try {
-		if ((flags & BLOCK_EFFECT) !== 0) {
+		if ((flags & (BLOCK_EFFECT | MANAGED_EFFECT)) !== 0) {
 			destroy_block_effect_children(effect);
 		} else {
 			destroy_effect_children(effect);

packages/svelte/tests/runtime-runes/samples/async-fork-attributes/_config.js

@@ -0,0 +1,60 @@
+import { tick } from 'svelte';
+import { test } from '../../test';
+
+export default test({
+	async test({ assert, target }) {
+		const [fork, commit] = target.querySelectorAll('button');
+
+		fork.click();
+		await tick();
+		assert.htmlEqual(
+			target.innerHTML,
+			`
+			<button>fork</button>
+			<button>commit</button>
+			<p style="">foo</p>
+			<p style="">foo</p>
+			<p>foo</p>
+		`
+		);
+
+		commit.click();
+		await tick();
+		assert.htmlEqual(
+			target.innerHTML,
+			`
+			<button>fork</button>
+			<button>commit</button>
+			<p style="color: red;">foo</p>
+			<p style="color: red;" data-attached=true>foo</p>
+			<p data-attached=true>foo</p>
+		`
+		);
+
+		fork.click();
+		await tick();
+		assert.htmlEqual(
+			target.innerHTML,
+			`
+			<button>fork</button>
+			<button>commit</button>
+			<p style="color: red;">foo</p>
+			<p style="color: red;" data-attached=true>foo</p>
+			<p data-attached=true>foo</p>
+		`
+		);
+
+		commit.click();
+		await tick();
+		assert.htmlEqual(
+			target.innerHTML,
+			`
+			<button>fork</button>
+			<button>commit</button>
+			<p style="">foo</p>
+			<p style="">foo</p>
+			<p>foo</p>
+		`
+		);
+	}
+});

packages/svelte/tests/runtime-runes/samples/async-fork-attributes/main.svelte

@@ -0,0 +1,28 @@
+<script>
+	import { fork } from "svelte";
+	import { createAttachmentKey } from "svelte/attachments";
+
+	let style = $state('');
+	let attach = $state(undefined);
+
+	let forked;
+</script>
+
+<button onclick={()=>{
+	forked = fork(()=>{
+		style = style ? '' : 'color: red';
+		attach = attach ? undefined : (node) => {
+			node.setAttribute('data-attached', 'true');
+			return () => node.removeAttribute('data-attached');
+		};
+	})
+}}>fork</button>
+
+<button onclick={()=>{
+	forked.commit();
+}}>commit</button>
+
+<!-- force $.attribute_effect, which uses a block effect -->
+<p {...{style}}>foo</p>
+<p {...{style, [createAttachmentKey()]: attach}}>foo</p>
+<p {@attach attach}>foo</p>