From 3c53da40ecfd66f9af3161bc445afa0e6a30c008 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Tue, 16 Apr 2024 21:09:29 +0100 Subject: [PATCH 1/3] fix: remove memory leak from retaining old DOM elements --- .changeset/rich-plums-thank.md | 5 ++ .../src/internal/client/dom/blocks/html.js | 17 +++- .../client/dom/blocks/svelte-element.js | 3 + .../src/internal/client/dom/template.js | 88 +++++++++++++++---- 4 files changed, 91 insertions(+), 22 deletions(-) create mode 100644 .changeset/rich-plums-thank.md diff --git a/.changeset/rich-plums-thank.md b/.changeset/rich-plums-thank.md new file mode 100644 index 000000000000..0c96191f9242 --- /dev/null +++ b/.changeset/rich-plums-thank.md @@ -0,0 +1,5 @@ +--- +"svelte": patch +--- + +fix: remove memory leak from retaining old DOM elements diff --git a/packages/svelte/src/internal/client/dom/blocks/html.js b/packages/svelte/src/internal/client/dom/blocks/html.js index ff96617885a7..b5deaf09dd40 100644 --- a/packages/svelte/src/internal/client/dom/blocks/html.js +++ b/packages/svelte/src/internal/client/dom/blocks/html.js @@ -1,8 +1,9 @@ import { derived } from '../../reactivity/deriveds.js'; import { render_effect } from '../../reactivity/effects.js'; -import { get } from '../../runtime.js'; +import { current_effect, get } from '../../runtime.js'; import { hydrate_nodes, hydrating } from '../hydration.js'; import { create_fragment_from_html, remove } from '../reconciler.js'; +import { push_template_node } from '../template.js'; /** * @param {Element | Text | Comment} anchor @@ -11,10 +12,12 @@ import { create_fragment_from_html, remove } from '../reconciler.js'; * @returns {void} */ export function html(anchor, get_value, svg) { + var effect = anchor.parentNode !== current_effect?.dom ? current_effect : null; let value = derived(get_value); render_effect(() => { - var dom = html_to_dom(anchor, get(value), svg); + var dom = html_to_dom(anchor, effect, get(value), svg); + effect = null; if (dom) { return () => remove(dom); @@ -27,11 +30,12 @@ export function html(anchor, get_value, svg) { * inserts it before the target anchor and returns the new nodes. * @template V * @param {Element | Text | Comment} target + * @param {import('#client').Effect | null} effect * @param {V} value * @param {boolean} svg * @returns {Element | Comment | (Element | Comment | Text)[]} */ -function html_to_dom(target, value, svg) { +function html_to_dom(target, effect, value, svg) { if (hydrating) return hydrate_nodes; var html = value + ''; @@ -49,6 +53,9 @@ function html_to_dom(target, value, svg) { if (node.childNodes.length === 1) { var child = /** @type {Text | Element | Comment} */ (node.firstChild); target.before(child); + if (effect !== null) { + push_template_node(effect, child); + } return child; } @@ -62,5 +69,9 @@ function html_to_dom(target, value, svg) { target.before(node); } + if (effect !== null) { + push_template_node(effect, nodes); + } + return nodes; } diff --git a/packages/svelte/src/internal/client/dom/blocks/svelte-element.js b/packages/svelte/src/internal/client/dom/blocks/svelte-element.js index a0c3cc6d6545..396bf55abf80 100644 --- a/packages/svelte/src/internal/client/dom/blocks/svelte-element.js +++ b/packages/svelte/src/internal/client/dom/blocks/svelte-element.js @@ -13,6 +13,7 @@ import { is_array } from '../../utils.js'; import { set_should_intro } from '../../render.js'; import { current_each_item, set_current_each_item } from './each.js'; import { current_effect } from '../../runtime.js'; +import { push_template_node } from '../template.js'; /** * @param {import('#client').Effect} effect @@ -131,6 +132,8 @@ export function element(anchor, get_tag, is_svg, render_fn) { if (prev_element) { swap_block_dom(parent_effect, prev_element, element); prev_element.remove(); + } else { + push_template_node(parent_effect, element); } }); } diff --git a/packages/svelte/src/internal/client/dom/template.js b/packages/svelte/src/internal/client/dom/template.js index 8e1f51c8eb1d..5baeb2f313de 100644 --- a/packages/svelte/src/internal/client/dom/template.js +++ b/packages/svelte/src/internal/client/dom/template.js @@ -4,6 +4,36 @@ import { create_fragment_from_html } from './reconciler.js'; import { current_effect } from '../runtime.js'; import { TEMPLATE_FRAGMENT, TEMPLATE_USE_IMPORT_NODE } from '../../../constants.js'; import { effect } from '../reactivity/effects.js'; +import { is_array } from '../utils.js'; + +/** + * @param {import("#client").Effect} effect + * @param {import("#client").TemplateNode | import("#client").TemplateNode[]} dom + */ +export function push_template_node(effect, dom) { + var current_dom = effect.dom; + if (current_dom === null) { + effect.dom = dom; + } else { + if (!is_array(current_dom)) { + current_dom = effect.dom = [current_dom]; + } + var anchor; + // If we're working with an anchor, then remove it and put it at the end. + if (current_dom[0].nodeType === 8) { + anchor = current_dom.pop(); + } + if (is_array(dom)) { + current_dom.push(...dom); + } else { + current_dom.push(dom); + } + if (anchor !== undefined) { + current_dom.push(anchor); + } + } + return dom; +} /** * @param {string} content @@ -19,16 +49,31 @@ export function template(content, flags) { var node; return () => { + var effect = /** @type {import('#client').Effect} */ (current_effect); if (hydrating) { - return is_fragment ? hydrate_nodes : /** @type {Node} */ (hydrate_nodes[0]); + var hydration_content = push_template_node( + effect, + is_fragment ? hydrate_nodes : hydrate_nodes[0] + ); + return /** @type {Node} */ (hydration_content); } if (!node) { node = create_fragment_from_html(content); if (!is_fragment) node = /** @type {Node} */ (node.firstChild); } + var clone = use_import_node ? document.importNode(node, true) : clone_node(node, true); + + if (is_fragment) { + push_template_node( + effect, + /** @type {import('#client').TemplateNode[]} */ ([...clone.childNodes]) + ); + } else { + push_template_node(effect, /** @type {import('#client').TemplateNode} */ (clone)); + } - return use_import_node ? document.importNode(node, true) : clone_node(node, true); + return clone; }; } @@ -70,8 +115,13 @@ export function svg_template(content, flags) { var node; return () => { + var effect = /** @type {import('#client').Effect} */ (current_effect); if (hydrating) { - return is_fragment ? hydrate_nodes : /** @type {Node} */ (hydrate_nodes[0]); + var hydration_content = push_template_node( + effect, + is_fragment ? hydrate_nodes : hydrate_nodes[0] + ); + return /** @type {Node} */ (hydration_content); } if (!node) { @@ -87,7 +137,18 @@ export function svg_template(content, flags) { } } - return clone_node(node, true); + var clone = clone_node(node, true); + + if (is_fragment) { + push_template_node( + effect, + /** @type {import('#client').TemplateNode[]} */ ([...clone.childNodes]) + ); + } else { + push_template_node(effect, /** @type {import('#client').TemplateNode} */ (clone)); + } + + return clone; }; } @@ -152,7 +213,8 @@ function run_scripts(node) { */ /*#__NO_SIDE_EFFECTS__*/ export function text(anchor) { - if (!hydrating) return empty(); + var effect = /** @type {import('#client').Effect} */ (current_effect); + if (!hydrating) return push_template_node(effect, empty()); var node = hydrate_nodes[0]; @@ -162,7 +224,7 @@ export function text(anchor) { anchor.before((node = empty())); } - return node; + return push_template_node(effect, node); } export const comment = template('', TEMPLATE_FRAGMENT); @@ -174,19 +236,7 @@ export const comment = template('', TEMPLATE_FRAGMENT); * @param {import('#client').Dom} dom */ export function append(anchor, dom) { - var current = dom; - if (!hydrating) { - var node = /** @type {Node} */ (dom); - - if (node.nodeType === 11) { - // if hydrating, `dom` is already an array of nodes, but if not then - // we need to create an array to store it on the current effect - current = /** @type {import('#client').Dom} */ ([...node.childNodes]); - } - - anchor.before(node); + anchor.before(/** @type {Node} */ (dom)); } - - /** @type {import('#client').Effect} */ (current_effect).dom = current; } From 1ade64ee182521f5442ff0ef504593744617ee09 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Tue, 16 Apr 2024 21:17:56 +0100 Subject: [PATCH 2/3] missing logic --- .../svelte/src/internal/client/dom/blocks/svelte-element.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/svelte/src/internal/client/dom/blocks/svelte-element.js b/packages/svelte/src/internal/client/dom/blocks/svelte-element.js index 396bf55abf80..c7daff227ba8 100644 --- a/packages/svelte/src/internal/client/dom/blocks/svelte-element.js +++ b/packages/svelte/src/internal/client/dom/blocks/svelte-element.js @@ -132,7 +132,7 @@ export function element(anchor, get_tag, is_svg, render_fn) { if (prev_element) { swap_block_dom(parent_effect, prev_element, element); prev_element.remove(); - } else { + } else if (!hydrating) { push_template_node(parent_effect, element); } }); From c2dfda15e0db88db4d27192e52807de0c7fd3e3a Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Wed, 17 Apr 2024 09:56:47 +0100 Subject: [PATCH 3/3] fix dynamic html bug --- .../src/internal/client/dom/blocks/html.js | 33 ++++++++++++++-- .../samples/each-dynamic-html/_config.js | 39 +++++++++++++++++++ .../samples/each-dynamic-html/main.svelte | 30 ++++++++++++++ 3 files changed, 98 insertions(+), 4 deletions(-) create mode 100644 packages/svelte/tests/runtime-runes/samples/each-dynamic-html/_config.js create mode 100644 packages/svelte/tests/runtime-runes/samples/each-dynamic-html/main.svelte diff --git a/packages/svelte/src/internal/client/dom/blocks/html.js b/packages/svelte/src/internal/client/dom/blocks/html.js index b5deaf09dd40..40583e09f242 100644 --- a/packages/svelte/src/internal/client/dom/blocks/html.js +++ b/packages/svelte/src/internal/client/dom/blocks/html.js @@ -1,10 +1,31 @@ import { derived } from '../../reactivity/deriveds.js'; import { render_effect } from '../../reactivity/effects.js'; import { current_effect, get } from '../../runtime.js'; +import { is_array } from '../../utils.js'; import { hydrate_nodes, hydrating } from '../hydration.js'; import { create_fragment_from_html, remove } from '../reconciler.js'; import { push_template_node } from '../template.js'; +/** + * @param {import('#client').Effect} effect + * @param {(Element | Comment | Text)[]} to_remove + * @returns {void} + */ +function remove_from_parent_effect(effect, to_remove) { + const dom = effect.dom; + + if (is_array(dom)) { + for (let i = dom.length - 1; i >= 0; i--) { + if (to_remove.includes(dom[i])) { + dom.splice(i, 1); + break; + } + } + } else if (dom !== null && to_remove.includes(dom)) { + effect.dom = null; + } +} + /** * @param {Element | Text | Comment} anchor * @param {() => string} get_value @@ -12,15 +33,19 @@ import { push_template_node } from '../template.js'; * @returns {void} */ export function html(anchor, get_value, svg) { - var effect = anchor.parentNode !== current_effect?.dom ? current_effect : null; + const parent_effect = anchor.parentNode !== current_effect?.dom ? current_effect : null; let value = derived(get_value); render_effect(() => { - var dom = html_to_dom(anchor, effect, get(value), svg); - effect = null; + var dom = html_to_dom(anchor, parent_effect, get(value), svg); if (dom) { - return () => remove(dom); + return () => { + if (parent_effect !== null) { + remove_from_parent_effect(parent_effect, is_array(dom) ? dom : [dom]); + } + remove(dom); + }; } }); } diff --git a/packages/svelte/tests/runtime-runes/samples/each-dynamic-html/_config.js b/packages/svelte/tests/runtime-runes/samples/each-dynamic-html/_config.js new file mode 100644 index 000000000000..93f370249108 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/each-dynamic-html/_config.js @@ -0,0 +1,39 @@ +import { flushSync } from '../../../../src/index-client'; +import { test } from '../../test'; + +export default test({ + html: ``, + + async test({ assert, target }) { + const [btn1, btn2, btn3] = target.querySelectorAll('button'); + + flushSync(() => { + btn1?.click(); + btn1?.click(); + btn1?.click(); + }); + + assert.htmlEqual( + target.innerHTML, + `
Item 1
Item 2
Item 3
` + ); + + flushSync(() => { + btn2?.click(); + }); + + assert.htmlEqual( + target.innerHTML, + `Item 1Item 2Item 3` + ); + + flushSync(() => { + btn3?.click(); + }); + + assert.htmlEqual( + target.innerHTML, + `Item 3Item 2Item 1` + ); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/each-dynamic-html/main.svelte b/packages/svelte/tests/runtime-runes/samples/each-dynamic-html/main.svelte new file mode 100644 index 000000000000..350cbf0d4aca --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/each-dynamic-html/main.svelte @@ -0,0 +1,30 @@ + + + + + + +{#each items as item (item.id)} + {@html item.html} +{/each}