From f7c8fd569b13a4af22d1c5929817be05c6511a6e Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Wed, 3 Apr 2024 10:35:13 -0400 Subject: [PATCH] Revert "chore: apply each block controlled teardown optimization (#11045)" (#11049) This reverts commit 1afec80261f09551b8daa664bd946f25c7a8b48a. --- .../src/internal/client/dom/blocks/each.js | 54 +++---------------- .../src/internal/client/reactivity/effects.js | 38 ++++++++++--- 2 files changed, 38 insertions(+), 54 deletions(-) diff --git a/packages/svelte/src/internal/client/dom/blocks/each.js b/packages/svelte/src/internal/client/dom/blocks/each.js index 6243f3360e9d..0ebbf442f701 100644 --- a/packages/svelte/src/internal/client/dom/blocks/each.js +++ b/packages/svelte/src/internal/client/dom/blocks/each.js @@ -15,11 +15,9 @@ import { untrack } from '../../runtime.js'; import { block, branch, - destroy_effect, effect, - run_out_transitions, - pause_children, pause_effect, + pause_effects, resume_effect } from '../../reactivity/effects.js'; import { source, mutable_source, set } from '../../reactivity/sources.js'; @@ -41,39 +39,6 @@ export function set_current_each_item(item) { current_each_item = item; } -/** - * Pause multiple effects simultaneously, and coordinate their - * subsequent destruction. Used in each blocks - * @param {import('#client').Effect[]} effects - * @param {null | Node} controlled_anchor - * @param {() => void} [callback] - */ -function pause_effects(effects, controlled_anchor, callback) { - /** @type {import('#client').TransitionManager[]} */ - var transitions = []; - var length = effects.length; - - for (var i = 0; i < length; i++) { - pause_children(effects[i], transitions, true); - } - - // If we have a controlled anchor, it means that the each block is inside a single - // DOM element, so we can apply a fast-path for clearing the contents of the element. - if (effects.length > 0 && transitions.length === 0 && controlled_anchor !== null) { - var parent_node = /** @type {Element} */ (controlled_anchor.parentNode); - parent_node.textContent = ''; - parent_node.append(controlled_anchor); - } - - run_out_transitions(transitions, () => { - for (var i = 0; i < length; i++) { - destroy_effect(effects[i]); - } - - if (callback !== undefined) callback(); - }); -} - /** * @template V * @param {Element | Comment} anchor The next sibling node, or the parent node if this is a 'controlled' block @@ -180,6 +145,7 @@ function each(anchor, flags, get_collection, get_key, render_fn, fallback_fn, re } if (!hydrating) { + // TODO add 'empty controlled block' optimisation here reconcile_fn(array, state, anchor, render_fn, flags, keys); } @@ -278,9 +244,7 @@ function reconcile_indexed_array(array, state, anchor, render_fn, flags) { effects.push(a_items[i].e); } - var controlled_anchor = (flags & EACH_IS_CONTROLLED) !== 0 && b === 0 ? anchor : null; - - pause_effects(effects, controlled_anchor, () => { + pause_effects(effects, () => { state.items.length = b; }); } @@ -310,7 +274,6 @@ function reconcile_tracked_array(array, state, anchor, render_fn, flags, keys) { var is_animated = (flags & EACH_IS_ANIMATED) !== 0; var should_update = (flags & (EACH_ITEM_REACTIVE | EACH_INDEX_REACTIVE)) !== 0; - var is_controlled = (flags & EACH_IS_CONTROLLED) !== 0; var start = 0; var item; @@ -418,11 +381,6 @@ function reconcile_tracked_array(array, state, anchor, render_fn, flags, keys) { // I fully understand this part) if (moved) { mark_lis(sources); - } else if (is_controlled && to_destroy.length === a_items.length) { - // We can optimize the case in which all items are replaced — - // destroy everything first, then append new items - pause_effects(to_destroy, anchor); - to_destroy = []; } // working from the back, insert new or moved items @@ -463,9 +421,9 @@ function reconcile_tracked_array(array, state, anchor, render_fn, flags, keys) { }); } - var controlled_anchor = is_controlled && b === 0 ? anchor : null; - - pause_effects(to_destroy, controlled_anchor, () => { + // TODO: would be good to avoid this closure in the case where we have no + // transitions at all. It would make it far more JIT friendly in the hot cases. + pause_effects(to_destroy, () => { state.items = b_items; }); } diff --git a/packages/svelte/src/internal/client/reactivity/effects.js b/packages/svelte/src/internal/client/reactivity/effects.js index 696504980143..4d54e4e887e0 100644 --- a/packages/svelte/src/internal/client/reactivity/effects.js +++ b/packages/svelte/src/internal/client/reactivity/effects.js @@ -27,6 +27,7 @@ import { IS_ELSEIF } from '../constants.js'; import { set } from './sources.js'; +import { noop } from '../../shared/utils.js'; import { remove } from '../dom/reconciler.js'; /** @@ -294,17 +295,42 @@ export function destroy_effect(effect) { * completed, and if the state change is reversed then we _resume_ it. * A paused effect does not update, and the DOM subtree becomes inert. * @param {import('#client').Effect} effect - * @param {() => void} [callback] + * @param {() => void} callback */ -export function pause_effect(effect, callback) { +export function pause_effect(effect, callback = noop) { /** @type {import('#client').TransitionManager[]} */ var transitions = []; pause_children(effect, transitions, true); - run_out_transitions(transitions, () => { + out(transitions, () => { destroy_effect(effect); - if (callback) callback(); + callback(); + }); +} + +/** + * Pause multiple effects simultaneously, and coordinate their + * subsequent destruction. Used in each blocks + * @param {import('#client').Effect[]} effects + * @param {() => void} callback + */ +export function pause_effects(effects, callback = noop) { + /** @type {import('#client').TransitionManager[]} */ + var transitions = []; + var length = effects.length; + + for (var i = 0; i < length; i++) { + pause_children(effects[i], transitions, true); + } + + // TODO: would be good to avoid this closure in the case where we have no + // transitions at all. It would make it far more JIT friendly in the hot cases. + out(transitions, () => { + for (var i = 0; i < length; i++) { + destroy_effect(effects[i]); + } + callback(); }); } @@ -312,7 +338,7 @@ export function pause_effect(effect, callback) { * @param {import('#client').TransitionManager[]} transitions * @param {() => void} fn */ -export function run_out_transitions(transitions, fn) { +function out(transitions, fn) { var remaining = transitions.length; if (remaining > 0) { var check = () => --remaining || fn(); @@ -329,7 +355,7 @@ export function run_out_transitions(transitions, fn) { * @param {import('#client').TransitionManager[]} transitions * @param {boolean} local */ -export function pause_children(effect, transitions, local) { +function pause_children(effect, transitions, local) { if ((effect.f & INERT) !== 0) return; effect.f ^= INERT;