From 8971910940ad654ca431d3b362a8579ad4628b68 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Fri, 29 Mar 2024 01:24:13 +0000 Subject: [PATCH] fix: further improvements to effect scheduling and flushing (#10971) * fix: improve effect scheduling * fix: further improvements to effect scheduling and flushin * add test * simplify * simplify * lint * fix e2e tests * fix e2e tests * simplify * Update packages/svelte/src/internal/client/runtime.js * Update packages/svelte/src/internal/client/runtime.js Co-authored-by: Rich Harris * Update packages/svelte/src/internal/client/runtime.js Co-authored-by: Rich Harris * Update packages/svelte/src/internal/client/runtime.js Co-authored-by: Rich Harris * style tweak --------- Co-authored-by: Rich Harris --- .changeset/brown-houses-obey.md | 5 + .../client/dom/elements/attributes.js | 6 +- .../internal/client/dom/legacy/lifecycle.js | 3 +- .../svelte/src/internal/client/runtime.js | 107 +++++++----------- .../samples/before-render-chain-2/Item.svelte | 24 ++++ .../samples/before-render-chain-2/_config.js | 11 ++ .../samples/before-render-chain-2/main.svelte | 9 ++ 7 files changed, 94 insertions(+), 71 deletions(-) create mode 100644 .changeset/brown-houses-obey.md create mode 100644 packages/svelte/tests/runtime-legacy/samples/before-render-chain-2/Item.svelte create mode 100644 packages/svelte/tests/runtime-legacy/samples/before-render-chain-2/_config.js create mode 100644 packages/svelte/tests/runtime-legacy/samples/before-render-chain-2/main.svelte diff --git a/.changeset/brown-houses-obey.md b/.changeset/brown-houses-obey.md new file mode 100644 index 0000000000..65c1ce4289 --- /dev/null +++ b/.changeset/brown-houses-obey.md @@ -0,0 +1,5 @@ +--- +"svelte": patch +--- + +fix: further improvements to effect scheduling and flushing diff --git a/packages/svelte/src/internal/client/dom/elements/attributes.js b/packages/svelte/src/internal/client/dom/elements/attributes.js index 8501756b24..9dd95bb78b 100644 --- a/packages/svelte/src/internal/client/dom/elements/attributes.js +++ b/packages/svelte/src/internal/client/dom/elements/attributes.js @@ -68,7 +68,11 @@ export function set_xlink_attribute(dom, attribute, value) { */ export function set_custom_element_data(node, prop, value) { if (prop in node) { - node[prop] = typeof node[prop] === 'boolean' && value === '' ? true : value; + var curr_val = node[prop]; + var next_val = typeof curr_val === 'boolean' && value === '' ? true : value; + if (typeof curr_val !== 'object' || curr_val !== next_val) { + node[prop] = next_val; + } } else { set_attribute(node, prop, value); } diff --git a/packages/svelte/src/internal/client/dom/legacy/lifecycle.js b/packages/svelte/src/internal/client/dom/legacy/lifecycle.js index fc7efbb006..c8a2b72d69 100644 --- a/packages/svelte/src/internal/client/dom/legacy/lifecycle.js +++ b/packages/svelte/src/internal/client/dom/legacy/lifecycle.js @@ -1,3 +1,4 @@ +import { CLEAN } from '../../constants.js'; import { run, run_all } from '../../../shared/utils.js'; import { user_pre_effect, user_effect } from '../../reactivity/effects.js'; import { @@ -27,7 +28,7 @@ export function init() { // 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. const parent = current_effect?.parent; - if (parent != null) { + if (parent != null && (parent.f & CLEAN) === 0) { flush_local_render_effects(parent); } }); diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js index 63d25ebe3a..0354e2d1aa 100644 --- a/packages/svelte/src/internal/client/runtime.js +++ b/packages/svelte/src/internal/client/runtime.js @@ -424,8 +424,7 @@ function infinite_loop_guard() { function flush_queued_root_effects(root_effects) { for (var i = 0; i < root_effects.length; i++) { var signal = root_effects[i]; - var effects = get_nested_effects(signal, RENDER_EFFECT | EFFECT); - flush_queued_effects(effects); + flush_nested_effects(signal, RENDER_EFFECT | EFFECT); } } @@ -438,24 +437,13 @@ function flush_queued_effects(effects) { if (length === 0) return; infinite_loop_guard(); - var previously_flushing_effect = is_flushing_effect; - is_flushing_effect = true; - - try { - for (var i = 0; i < length; i++) { - var signal = effects[i]; + for (var i = 0; i < length; i++) { + var effect = effects[i]; - if ((signal.f & (DESTROYED | INERT)) === 0) { - if (check_dirtiness(signal)) { - execute_effect(signal); - } - } + if ((effect.f & (DESTROYED | INERT)) === 0 && check_dirtiness(effect)) { + execute_effect(effect); } - } finally { - is_flushing_effect = previously_flushing_effect; } - - effects.length = 0; } function process_microtask() { @@ -512,69 +500,49 @@ export function schedule_effect(signal) { * @param {import('./types.js').Effect} effect * @param {number} filter_flags * @param {boolean} shallow - * @param {import('./types.js').Effect[]} collected_render * @param {import('./types.js').Effect[]} collected_user * @returns {void} */ -function recursively_collect_effects( - effect, - filter_flags, - shallow, - collected_render, - collected_user -) { +function recursively_process_effects(effect, filter_flags, shallow, collected_user) { var effects = effect.effects; if (effects === null) return; - var render = []; var user = []; for (var i = 0; i < effects.length; i++) { var child = effects[i]; var flags = child.f; + var is_inactive = (flags & (DESTROYED | INERT)) !== 0; + if (is_inactive) continue; var is_branch = flags & BRANCH_EFFECT; + var is_clean = (flags & CLEAN) !== 0; if (is_branch) { // Skip this branch if it's clean - if ((flags & CLEAN) !== 0) continue; + if (is_clean) continue; set_signal_status(child, CLEAN); } if ((flags & RENDER_EFFECT) !== 0) { if (is_branch) { if (shallow) continue; - recursively_collect_effects(child, filter_flags, false, collected_render, collected_user); + recursively_process_effects(child, filter_flags, false, collected_user); } else { - render.push(child); + if (check_dirtiness(child)) { + execute_effect(child); + } + recursively_process_effects(child, filter_flags, false, collected_user); } } else if ((flags & EFFECT) !== 0) { - if (is_branch) { + if (is_branch || is_clean) { if (shallow) continue; - recursively_collect_effects(child, filter_flags, false, collected_render, collected_user); + recursively_process_effects(child, filter_flags, false, collected_user); } else { user.push(child); } } } - if (render.length > 0) { - if ((filter_flags & RENDER_EFFECT) !== 0) { - collected_render.push(...render); - } - - if (!shallow) { - for (i = 0; i < render.length; i++) { - recursively_collect_effects( - render[i], - filter_flags, - false, - collected_render, - collected_user - ); - } - } - } - if (user.length > 0) { if ((filter_flags & EFFECT) !== 0) { collected_user.push(...user); @@ -582,7 +550,7 @@ function recursively_collect_effects( if (!shallow) { for (i = 0; i < user.length; i++) { - recursively_collect_effects(user[i], filter_flags, false, collected_render, collected_user); + recursively_process_effects(user[i], filter_flags, false, collected_user); } } } @@ -597,23 +565,26 @@ function recursively_collect_effects( * @param {import('./types.js').Effect} effect * @param {number} filter_flags * @param {boolean} [shallow] - * @returns {import('./types.js').Effect[]} + * @returns {void} */ -function get_nested_effects(effect, filter_flags, shallow = false) { - /** - * @type {import("./types.js").Effect[]} - */ - var render_effects = []; - /** - * @type {import("./types.js").Effect[]} - */ +function flush_nested_effects(effect, filter_flags, shallow = false) { + /** @type {import('#client').Effect[]} */ var user_effects = []; - // When working with custom-elements, the root effects might not have a root - if (effect.effects === null && (effect.f & BRANCH_EFFECT) === 0) { - return [effect]; + + var previously_flushing_effect = is_flushing_effect; + is_flushing_effect = true; + + try { + // When working with custom elements, the root effects might not have a root + if (effect.effects === null && (effect.f & BRANCH_EFFECT) === 0) { + flush_queued_effects([effect]); + } else { + recursively_process_effects(effect, filter_flags, shallow, user_effects); + flush_queued_effects(user_effects); + } + } finally { + is_flushing_effect = previously_flushing_effect; } - recursively_collect_effects(effect, filter_flags, shallow, render_effects, user_effects); - return [...render_effects, ...user_effects]; } /** @@ -621,11 +592,9 @@ function get_nested_effects(effect, filter_flags, shallow = false) { * @returns {void} */ export function flush_local_render_effects(effect) { - /** - * @type {import("./types.js").Effect[]} - */ - var render_effects = get_nested_effects(effect, RENDER_EFFECT, true); - flush_queued_effects(render_effects); + // We are entering a new flush sequence, so ensure counter is reset. + flush_count = 0; + flush_nested_effects(effect, RENDER_EFFECT, true); } /** diff --git a/packages/svelte/tests/runtime-legacy/samples/before-render-chain-2/Item.svelte b/packages/svelte/tests/runtime-legacy/samples/before-render-chain-2/Item.svelte new file mode 100644 index 0000000000..5e747768e9 --- /dev/null +++ b/packages/svelte/tests/runtime-legacy/samples/before-render-chain-2/Item.svelte @@ -0,0 +1,24 @@ + + + diff --git a/packages/svelte/tests/runtime-legacy/samples/before-render-chain-2/_config.js b/packages/svelte/tests/runtime-legacy/samples/before-render-chain-2/_config.js new file mode 100644 index 0000000000..e71ef46849 --- /dev/null +++ b/packages/svelte/tests/runtime-legacy/samples/before-render-chain-2/_config.js @@ -0,0 +1,11 @@ +import { flushSync } from 'svelte'; +import { test } from '../../test'; + +export default test({ + async test({ assert, component, target }) { + const [btn1] = target.querySelectorAll('button'); + flushSync(() => { + // This test would result in an infinite loop, so if this doesn't error, then the test is working. + }); + } +}); diff --git a/packages/svelte/tests/runtime-legacy/samples/before-render-chain-2/main.svelte b/packages/svelte/tests/runtime-legacy/samples/before-render-chain-2/main.svelte new file mode 100644 index 0000000000..908d07f39a --- /dev/null +++ b/packages/svelte/tests/runtime-legacy/samples/before-render-chain-2/main.svelte @@ -0,0 +1,9 @@ + + +{#each items as item} + +{/each}