From b3946e05d63740335465b26fde21920e833ebf1d Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Wed, 13 Mar 2024 09:39:47 +0000 Subject: [PATCH 1/5] simplify pre_effect logic --- .../src/internal/client/dom/legacy/lifecycle.js | 3 ++- .../svelte/src/internal/client/reactivity/effects.js | 11 +---------- 2 files changed, 3 insertions(+), 11 deletions(-) diff --git a/packages/svelte/src/internal/client/dom/legacy/lifecycle.js b/packages/svelte/src/internal/client/dom/legacy/lifecycle.js index d7ec2efa08cb..50342dfb85ed 100644 --- a/packages/svelte/src/internal/client/dom/legacy/lifecycle.js +++ b/packages/svelte/src/internal/client/dom/legacy/lifecycle.js @@ -1,6 +1,6 @@ import { run } from '../../../common.js'; import { pre_effect, user_effect } from '../../reactivity/effects.js'; -import { current_component_context, deep_read_state, get, untrack } from '../../runtime.js'; +import { current_component_context, deep_read_state, flush_local_render_effects, get, untrack } from '../../runtime.js'; /** * Legacy-mode only: Call `onMount` callbacks and set up `beforeUpdate`/`afterUpdate` effects @@ -16,6 +16,7 @@ export function init() { pre_effect(() => { observe_all(context); callbacks.b.forEach(run); + flush_local_render_effects(); }); } diff --git a/packages/svelte/src/internal/client/reactivity/effects.js b/packages/svelte/src/internal/client/reactivity/effects.js index 8875e03ded05..99c98013d498 100644 --- a/packages/svelte/src/internal/client/reactivity/effects.js +++ b/packages/svelte/src/internal/client/reactivity/effects.js @@ -148,18 +148,9 @@ export function pre_effect(fn) { : '') ); } - const sync = current_effect !== null && (current_effect.f & RENDER_EFFECT) !== 0; - return create_effect( - PRE_EFFECT, - () => { - const val = fn(); - flush_local_render_effects(); - return val; - }, - sync - ); + return create_effect(PRE_EFFECT, fn, sync); } /** From 087c573e4e1d07991625210366894f240189855c Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Wed, 13 Mar 2024 09:54:38 +0000 Subject: [PATCH 2/5] fix test --- .../svelte/src/internal/client/dom/legacy/lifecycle.js | 8 +++++++- packages/svelte/src/reactivity/set.js | 2 +- packages/svelte/src/reactivity/set.test.ts | 4 +--- 3 files changed, 9 insertions(+), 5 deletions(-) diff --git a/packages/svelte/src/internal/client/dom/legacy/lifecycle.js b/packages/svelte/src/internal/client/dom/legacy/lifecycle.js index 50342dfb85ed..c77bbe08407c 100644 --- a/packages/svelte/src/internal/client/dom/legacy/lifecycle.js +++ b/packages/svelte/src/internal/client/dom/legacy/lifecycle.js @@ -1,6 +1,12 @@ import { run } from '../../../common.js'; import { pre_effect, user_effect } from '../../reactivity/effects.js'; -import { current_component_context, deep_read_state, flush_local_render_effects, get, untrack } from '../../runtime.js'; +import { + current_component_context, + deep_read_state, + flush_local_render_effects, + get, + untrack +} from '../../runtime.js'; /** * Legacy-mode only: Call `onMount` callbacks and set up `beforeUpdate`/`afterUpdate` effects diff --git a/packages/svelte/src/reactivity/set.js b/packages/svelte/src/reactivity/set.js index 5ec8193f0e09..13cf24849351 100644 --- a/packages/svelte/src/reactivity/set.js +++ b/packages/svelte/src/reactivity/set.js @@ -82,9 +82,9 @@ export class ReactiveSet extends Set { /** @param {T} value */ has(value) { var source = this.#sources.get(value); + get(this.#version); if (source === undefined) { - get(this.#version); return false; } diff --git a/packages/svelte/src/reactivity/set.test.ts b/packages/svelte/src/reactivity/set.test.ts index 625a90db7d13..cea7221060ba 100644 --- a/packages/svelte/src/reactivity/set.test.ts +++ b/packages/svelte/src/reactivity/set.test.ts @@ -30,9 +30,7 @@ test('set.values()', () => { set.clear(); }); - // TODO looks like another effect ordering bug — sequence should be , - // but values is reversed at end - assert.deepEqual(log, [5, true, [1, 2, 3, 4, 5], 4, false, [1, 2, 4, 5], 0, [], false]); + assert.deepEqual(log, [5, true, [1, 2, 3, 4, 5], 4, false, [1, 2, 4, 5], 0, false, []]); cleanup(); }); From 8e8224c008c303004e6599f2faab62812aa76c91 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Wed, 13 Mar 2024 09:56:32 +0000 Subject: [PATCH 3/5] add changeset and test --- .changeset/tasty-steaks-smile.md | 5 +++++ .../_config.js | 21 +++++++++++++++++++ .../log.js | 2 ++ .../main.svelte | 19 +++++++++++++++++ 4 files changed, 47 insertions(+) create mode 100644 .changeset/tasty-steaks-smile.md create mode 100644 packages/svelte/tests/runtime-runes/samples/lifecycle-render-order-for-children-5/_config.js create mode 100644 packages/svelte/tests/runtime-runes/samples/lifecycle-render-order-for-children-5/log.js create mode 100644 packages/svelte/tests/runtime-runes/samples/lifecycle-render-order-for-children-5/main.svelte diff --git a/.changeset/tasty-steaks-smile.md b/.changeset/tasty-steaks-smile.md new file mode 100644 index 000000000000..38ece02f2606 --- /dev/null +++ b/.changeset/tasty-steaks-smile.md @@ -0,0 +1,5 @@ +--- +"svelte": patch +--- + +fix: adjust render effect ordering diff --git a/packages/svelte/tests/runtime-runes/samples/lifecycle-render-order-for-children-5/_config.js b/packages/svelte/tests/runtime-runes/samples/lifecycle-render-order-for-children-5/_config.js new file mode 100644 index 000000000000..bc62b4f2c922 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/lifecycle-render-order-for-children-5/_config.js @@ -0,0 +1,21 @@ +import { test } from '../../test'; +import { log } from './log.js'; + +export default test({ + get props() { + return { n: 0 }; + }, + + before_test() { + log.length = 0; + }, + + async test({ assert, component }) { + assert.deepEqual(log, ['$effect.pre 0', 'another $effect.pre 1', 'render n0', 'render i1']); + + log.length = 0; + component.n += 1; + + assert.deepEqual(log, ['$effect.pre 1', 'another $effect.pre 2', 'render n1', 'render i2']); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/lifecycle-render-order-for-children-5/log.js b/packages/svelte/tests/runtime-runes/samples/lifecycle-render-order-for-children-5/log.js new file mode 100644 index 000000000000..d3df521f4da7 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/lifecycle-render-order-for-children-5/log.js @@ -0,0 +1,2 @@ +/** @type {any[]} */ +export const log = []; diff --git a/packages/svelte/tests/runtime-runes/samples/lifecycle-render-order-for-children-5/main.svelte b/packages/svelte/tests/runtime-runes/samples/lifecycle-render-order-for-children-5/main.svelte new file mode 100644 index 000000000000..ecbd9321bf00 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/lifecycle-render-order-for-children-5/main.svelte @@ -0,0 +1,19 @@ + + +

{logRender(`n${n}`)}

+

{logRender(`i${i}`)}

From fda33809e88f54cd44dcd329bc39ac7dac0463b2 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Wed, 13 Mar 2024 10:00:43 +0000 Subject: [PATCH 4/5] add comment --- packages/svelte/src/reactivity/set.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/svelte/src/reactivity/set.js b/packages/svelte/src/reactivity/set.js index 13cf24849351..dcd544671d71 100644 --- a/packages/svelte/src/reactivity/set.js +++ b/packages/svelte/src/reactivity/set.js @@ -82,6 +82,8 @@ export class ReactiveSet extends Set { /** @param {T} value */ has(value) { var source = this.#sources.get(value); + // We should always track the version in case + // the Set ever gets this value in the future. get(this.#version); if (source === undefined) { From c73f099524200b6c47967257fe881ef735381dcf Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Wed, 13 Mar 2024 10:05:26 +0000 Subject: [PATCH 5/5] Update packages/svelte/src/internal/client/dom/legacy/lifecycle.js Co-authored-by: Simon H <5968653+dummdidumm@users.noreply.github.com> --- packages/svelte/src/internal/client/dom/legacy/lifecycle.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/svelte/src/internal/client/dom/legacy/lifecycle.js b/packages/svelte/src/internal/client/dom/legacy/lifecycle.js index c77bbe08407c..425f65398a93 100644 --- a/packages/svelte/src/internal/client/dom/legacy/lifecycle.js +++ b/packages/svelte/src/internal/client/dom/legacy/lifecycle.js @@ -22,6 +22,9 @@ export function init() { pre_effect(() => { observe_all(context); callbacks.b.forEach(run); + // beforeUpdate might change state that affects rendering, ensure the render effects following from it + // are batched up with the current run. Avoids for example child components rerunning when they're + // now hidden because beforeUpdate did set an if block to false. flush_local_render_effects(); }); }