Skip to content

chore: simplify effect.dom stuff #11752

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 23, 2024
Merged

chore: simplify effect.dom stuff #11752

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 2 additions & 11 deletions packages/svelte/src/internal/client/dom/blocks/html.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ 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
Expand Down Expand Up @@ -38,7 +37,7 @@ export function html(anchor, get_value, svg, mathml) {
let value = derived(get_value);

render_effect(() => {
var dom = html_to_dom(anchor, parent_effect, get(value), svg, mathml);
var dom = html_to_dom(anchor, get(value), svg, mathml);

if (dom) {
return () => {
Expand All @@ -56,13 +55,12 @@ export function html(anchor, get_value, svg, mathml) {
* 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
* @param {boolean} mathml
* @returns {Element | Comment | (Element | Comment | Text)[]}
*/
function html_to_dom(target, effect, value, svg, mathml) {
function html_to_dom(target, value, svg, mathml) {
if (hydrating) return hydrate_nodes;

var html = value + '';
Expand All @@ -81,9 +79,6 @@ function html_to_dom(target, effect, value, svg, mathml) {
if (node.childNodes.length === 1) {
var child = /** @type {Text | Element | Comment} */ (node.firstChild);
target.before(child);
if (effect !== null) {
push_template_node(child, effect);
}
return child;
}

Expand All @@ -97,9 +92,5 @@ function html_to_dom(target, effect, value, svg, mathml) {
target.before(node);
}

if (effect !== null) {
push_template_node(nodes, effect);
}

return nodes;
}
38 changes: 4 additions & 34 deletions packages/svelte/src/internal/client/dom/blocks/svelte-element.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,37 +6,13 @@ import {
branch,
destroy_effect,
pause_effect,
render_effect,
resume_effect
} from '../../reactivity/effects.js';
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_component_context, current_effect } from '../../runtime.js';
import { push_template_node } from '../template.js';
import { current_component_context } from '../../runtime.js';
import { DEV } from 'esm-env';

/**
* @param {import('#client').Effect} effect
* @param {Element} from
* @param {Element} to
* @returns {void}
*/
function swap_block_dom(effect, from, to) {
const dom = effect.dom;

if (is_array(dom)) {
for (let i = 0; i < dom.length; i++) {
if (dom[i] === from) {
dom[i] = to;
break;
}
}
} else if (dom === from) {
effect.dom = to;
}
}

/**
* @param {Comment} anchor
* @param {() => string} get_tag
Expand All @@ -47,8 +23,6 @@ function swap_block_dom(effect, from, to) {
* @returns {void}
*/
export function element(anchor, get_tag, is_svg, render_fn, get_namespace, location) {
const parent_effect = /** @type {import('#client').Effect} */ (current_effect);

const filename = DEV && location && current_component_context?.function.filename;

/** @type {string | null} */
Expand Down Expand Up @@ -104,7 +78,6 @@ export function element(anchor, get_tag, is_svg, render_fn, get_namespace, locat

if (next_tag && next_tag !== current_tag) {
effect = branch(() => {
const prev_element = element;
element = hydrating
? /** @type {Element} */ (hydrate_nodes[0])
: ns
Expand Down Expand Up @@ -144,12 +117,9 @@ export function element(anchor, get_tag, is_svg, render_fn, get_namespace, locat

anchor.before(element);

if (prev_element) {
swap_block_dom(parent_effect, prev_element, element);
prev_element.remove();
} else if (!hydrating) {
push_template_node(element, parent_effect);
}
return () => {
element?.remove();
};
});
}

Expand Down
58 changes: 19 additions & 39 deletions packages/svelte/src/internal/client/dom/template.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,27 +9,15 @@ import { is_array } from '../utils.js';
/**
* @template {import("#client").TemplateNode | import("#client").TemplateNode[]} T
* @param {T} dom
* @param {import("#client").Effect} effect
* @returns {T}
*/
export function push_template_node(
dom,
effect = /** @type {import('#client').Effect} */ (current_effect)
) {
var current_dom = effect.dom;
if (current_dom === null) {
effect.dom = dom;
} else {
if (!is_array(current_dom)) {
current_dom = effect.dom = [current_dom];
}
function push_template_node(dom) {
var effect = /** @type {import('#client').Effect} */ (current_effect);

if (is_array(dom)) {
current_dom.push(...dom);
} else {
current_dom.push(dom);
}
if (effect.dom === null) {
effect.dom = dom;
}

return dom;
}

Expand All @@ -55,15 +43,8 @@ export function template(content, flags) {
node = create_fragment_from_html(content);
if (!is_fragment) node = /** @type {Node} */ (node.firstChild);
}
var clone = use_import_node ? document.importNode(node, true) : node.cloneNode(true);

push_template_node(
is_fragment
? /** @type {import('#client').TemplateNode[]} */ ([...clone.childNodes])
: /** @type {import('#client').TemplateNode} */ (clone)
);

return clone;
return use_import_node ? document.importNode(node, true) : node.cloneNode(true);
};
}

Expand Down Expand Up @@ -123,15 +104,7 @@ export function ns_template(content, flags, ns = 'svg') {
}
}

var clone = node.cloneNode(true);

push_template_node(
is_fragment
? /** @type {import('#client').TemplateNode[]} */ ([...clone.childNodes])
: /** @type {import('#client').TemplateNode} */ (clone)
);

return clone;
return node.cloneNode(true);
};
}

Expand Down Expand Up @@ -206,7 +179,7 @@ function run_scripts(node) {
*/
/*#__NO_SIDE_EFFECTS__*/
export function text(anchor) {
if (!hydrating) return push_template_node(empty());
if (!hydrating) return empty();

var node = hydrate_nodes[0];

Expand All @@ -227,7 +200,6 @@ export function comment() {
var frag = document.createDocumentFragment();
var anchor = empty();
frag.append(anchor);
push_template_node([anchor]);

return frag;
}
Expand All @@ -236,10 +208,18 @@ export function comment() {
* Assign the created (or in hydration mode, traversed) dom elements to the current block
* and insert the elements into the dom (in client mode).
* @param {Text | Comment | Element} anchor
* @param {import('#client').Dom} dom
* @param {DocumentFragment | Element | Comment} node
*/
export function append(anchor, dom) {
export function append(anchor, node) {
if (!hydrating) {
anchor.before(/** @type {Node} */ (dom));
/** @type {import('#client').Dom} */
const dom =
node.nodeType === 11
? /** @type {import('#client').TemplateNode[]} */ ([...node.childNodes])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to cause a memory leak. We shouldn't reference all child nodes, as those nodes might get removed from internal effects – yet still get referenced here and thus retained. I should have noticed this on my review, sigh.

: /** @type {Element | Comment} */ (node);

/** @type {import('#client').Effect} */ (current_effect).dom = dom;

anchor.before(/** @type {Node} */ (node));
}
}
Loading