diff --git a/.changeset/spread-fewer-nodes.md b/.changeset/spread-fewer-nodes.md new file mode 100644 index 000000000..e7c07ac9a --- /dev/null +++ b/.changeset/spread-fewer-nodes.md @@ -0,0 +1,5 @@ +--- +"@solidjs/web": patch +--- + +`spread()` creates fewer reactive nodes per element (#3388): `ref` folds into the attribute effect and is re-applied only when its identity changes (refs run with no owner, so nothing they create is disposed by the fold); children keep their own owned `insert` — that effect owns the child subtree, and merging it would rebuild the children on every attribute change — but a plain object whose `children` is a data property inserts the value with no effect at all. Three nodes become two when children flow through the spread, one when they don't. `spread` also accepts an array of sources with an optional `skip` predicate — `spread(el, [a, b], skipChildren, skip)` — the union of own keys with later sources winning, only the winning source read, function sources called inline with no memo (and so no hydration id), matching the server `ssrElement` array form. diff --git a/packages/web/src/client.ts b/packages/web/src/client.ts index 83ef2657a..049cad6ed 100644 --- a/packages/web/src/client.ts +++ b/packages/web/src/client.ts @@ -825,67 +825,143 @@ export function setStyleProperty(node, name, value) { if (isHydrating(node)) return; value != null ? node.style.setProperty(name, value) : node.style.removeProperty(name); } /** Compiler-emitted primitive; not for hand-written code. @internal */ -export function spread(node: Element, accessor: T, skipChildren?: Boolean): void; +export function spread( + node: Element, + sources: unknown[], + skipChildren?: Boolean, + skip?: (key: string) => boolean +): void; +export function spread( + node: Element, + accessor: T, + skipChildren?: Boolean, + skip?: (key: string) => boolean +): void; -// TODO: make this better -export function spread(node, props, skipChildren) { +// At most TWO reactive nodes per element (#3388) — one when nothing flows +// through `children`: +// +// - The children `insert` effect stays separate. It OWNS the child subtree: +// components, memos and effects created while the children getter runs are +// disposed when it reruns, so folding it into the attribute effect would +// tear the children down and rebuild them on every attribute change. When +// the source is a plain object (no accessor, no proxy) whose `children` is +// a data property, the value is inserted directly — `insert` with a +// non-function creates no effect at all. Compiled JSX children are getters +// and keep the effect path. +// - `ref` FOLDS into the attribute effect. It is collected with the other +// props in the compute half and applied in the commit half only when its +// identity differs from the last applied one (`prevProps.ref`, recorded by +// assign()). `ref()` runs the callback untracked with NO owner — refs +// deliberately own nothing — so anything a ref callback creates survives +// the effect rerunning; that is what makes the fold safe. +// +// Sources. A single source is an object, a merge() result or a bare +// accessor. A lone reactive spread compiles to its accessor directly: merging +// one source is pure overhead, and the mergeProps memo would consume a +// hydration id the server-side fast path never allocates (#3105). The +// accessor resolves inside each tracking scope instead. A nullish source +// (`{...props()}` where the optional props are absent, or no source at all) +// is an empty spread: attributes applied by the previous value are removed, +// nothing throws (#3297). +// +// An ARRAY of sources is the union of their own string keys, later sources +// winning per key (Object.assign / merge()'s contract); only the winning +// source's value is read, so a shadowed getter never runs. A function source +// is called inline in the compute half, tracked, once per run — NO memo and +// so NO hydration id, matching the server's `ssrElement` array form. Nullish +// sources are skipped. `skip(key)` → the key is never read nor applied. +export function spread(node, props, skipChildren, skip) { const prevProps = {}; - // A lone reactive spread compiles to its accessor directly: merging one - // source is pure overhead, and the mergeProps memo would consume a - // hydration id the server-side fast path never allocates (#3105). The - // accessor resolves inside each tracking scope instead. A nullish source - // (`{...props()}` where the optional props are absent, or no source at all) - // is an empty spread: attributes applied by the previous value are removed, - // nothing throws (#3297). - const get = () => (typeof props === "function" ? props() : props) ?? {}; - if (!skipChildren) - insert(node, () => { - const source = get(); - return hasOwn.call(source, "children") ? source.children : undefined; - }); - effect( - () => { - const source = get(); - const r = hasOwn.call(source, "ref") && source.ref; - (typeof r === "function" || Array.isArray(r)) && ref(() => r, node); - }, - () => {} - ); - effect( - () => { - const source = get(); - const newProps = {}; - // A merge() proxy is read through its SOURCES, not through the proxy: a - // spread mixed with other attributes compiles to - // `spread(el, merge(statics, () => rest))`, and going through the proxy - // costs merge's `keys()` (a Set plus an own-enumerable scan of every - // source) and then, per key, a right-to-left `in` walk of the sources. - // The union of own string keys with later sources overriding earlier - // — Object.assign order, merge's own contract — is all a spread needs. - // omit() is not a merge: it stays a proxy and is enumerated through its - // own filtering trap. - const sources = mergeSources(source); - if (sources !== undefined) { - for (let i = 0; i < sources.length; i++) { - let s = sources[i]; - if (typeof s === "function") s = s(); - if (s != null) collectProps(newProps, s); + const apply = newProps => { + const r = newProps.ref; + if (r !== prevProps.ref && (typeof r === "function" || Array.isArray(r))) ref(() => r, node); + assign(node, newProps, true, prevProps, true); + }; + if (Array.isArray(props)) { + if (!skipChildren && !(skip !== undefined && skip("children"))) + insert(node, () => { + for (let i = props.length - 1; i >= 0; i--) { + const s = resolveSource(props[i]); + if (s != null && "children" in s) return s.children; } - } else collectProps(newProps, source); - return newProps; - }, - props => assign(node, props, true, prevProps, true) - ); + }); + effect(() => collectSources({}, props, skip), apply); + return prevProps; + } + if (!skipChildren && !(skip !== undefined && skip("children"))) { + if (typeof props !== "function" && props != null && props[$PROXY] !== props) { + // A plain object's key set can't change reactively: no `children` key + // means nothing to insert, a data property inserts its value with no + // effect, only a getter needs the tracking scope. + const desc = Object.getOwnPropertyDescriptor(props, "children"); + if (desc !== undefined) { + if (desc.get === undefined) insert(node, desc.value); + else insert(node, () => props.children); + } + } else + insert(node, () => { + const source = resolveSource(props); + return source != null && hasOwn.call(source, "children") ? source.children : undefined; + }); + } + effect(() => { + const source = resolveSource(props); + const newProps = {}; + // A merge() proxy is read through its SOURCES, not through the proxy: a + // spread mixed with other attributes compiles to + // `spread(el, merge(statics, () => rest))`, and going through the proxy + // costs merge's `keys()` (a Set plus an own-enumerable scan of every + // source) and then, per key, a right-to-left `in` walk of the sources. + // The union of own string keys with later sources overriding earlier + // — Object.assign order, merge's own contract — is all a spread needs. + // omit() is not a merge: it stays a proxy and is enumerated through its + // own filtering trap. + const sources = mergeSources(source); + if (sources !== undefined) return collectSources(newProps, sources, skip); + if (source != null) collectProps(newProps, source, skip); + return newProps; + }, apply); return prevProps; } -// One layer of a spread source into `out`: own string keys, children/ref -// excluded, object-valued style/class read HERE, tracked (see readShallow()). -function collectProps(out, s) { +function resolveSource(s) { + return typeof s === "function" ? s() : s; +} + +// Layered sources into `out`. Every function source is resolved once, up +// front; keys are then collected left-to-right (Object.assign order — the +// order assign() applies them in, which `type`/`value`/`min`/`max` style +// pairs care about), and a key any LATER source has is skipped unread. `in` +// is merge()'s own resolution test, so a proxy source (store, omit) answers +// through its `has` trap rather than a per-key descriptor trap. +function collectSources(out, sources, skip) { + const n = sources.length; + const resolved = new Array(n); + for (let i = 0; i < n; i++) resolved[i] = resolveSource(sources[i]); + for (let i = 0; i < n; i++) { + const s = resolved[i]; + if (s != null) collectProps(out, s, skip, resolved, i + 1); + } + return out; +} + +// One layer of a spread source into `out`: own string keys, `children` +// excluded (it has its own insert), `ref` carried through for the commit +// half, object-valued style/class read HERE, tracked (see readShallow()). +// With `later` (the sources after this one, from index `from`), a key one of +// them defines is shadowed and never read here. +function collectProps(out, s, skip, later?, from?) { const keys = ownKeys(s); - for (let i = 0; i < keys.length; i++) { + outer: for (let i = 0; i < keys.length; i++) { const prop = keys[i]; - if (typeof prop !== "string" || prop === "children" || prop === "ref") continue; + if (typeof prop !== "string" || prop === "children") continue; + if (skip !== undefined && skip(prop)) continue; + if (later !== undefined) + for (let j = from; j < later.length; j++) { + const t = later[j]; + if (t != null && prop in t) continue outer; + } const v = s[prop]; out[prop] = prop === "style" || prop === "class" ? readShallow(v) : v; } diff --git a/packages/web/test/spread-nodes.spec.tsx b/packages/web/test/spread-nodes.spec.tsx new file mode 100644 index 000000000..e26b7e687 --- /dev/null +++ b/packages/web/test/spread-nodes.spec.tsx @@ -0,0 +1,451 @@ +/** + * @jsxImportSource @solidjs/web + * @vitest-environment jsdom + * + * spread() creates at most two reactive nodes per element (#3388): the + * children `insert` effect, which OWNS the child subtree and so must stay + * separate, and one attribute effect that also carries `ref` (refs own + * nothing — `ref()` applies them with no owner — so folding them in is safe). + * Plain data `children` insert with no effect at all. The array form + * `spread(el, [a, () => b, c], skipChildren, skip)` unions own keys with + * later sources winning, reads only the winner, calls function sources + * inline (no memo, no hydration id) and honors a `skip` predicate. + */ +import { describe, expect, test, vi } from "vitest"; +import { render, spread } from "@solidjs/web"; +import { createRoot, createSignal, flush, getOwner, merge } from "solid-js"; + +const mount = (el: () => any) => { + const container = document.createElement("div"); + const dispose = render(el, container); + flush(); + return { container, dispose, el: () => container.firstElementChild as HTMLElement }; +}; + +function ownerTotal(node: any): number { + let count = 0; + for (let s = node._firstChild; s; s = s._nextSibling) count += 1 + ownerTotal(s); + return count; +} + +/** Reactive nodes created under a root by building the source (a merge() memo + * counts) and one `spread(...)` call over it. */ +function spreadNodes(source: () => any, skipChildren?: boolean) { + let count = -1; + const dispose = createRoot(d => { + const owner = getOwner(); + const el = document.createElement("div"); + spread(el, source(), skipChildren); + flush(); + count = ownerTotal(owner); + return d; + }); + dispose(); + return count; +} + +describe("reactive node count per element", () => { + test("attributes + getter children: 2 nodes (children insert + attribute effect)", () => { + const [title] = createSignal("t"); + const props = { + get title() { + return title(); + }, + get children() { + return "text"; + } + }; + expect(spreadNodes(() => props)).toBe(2); + // Lone reactive spread: the accessor itself, no memo (#3105). + expect(spreadNodes(() => () => props)).toBe(2); + }); + + test("attributes without children: 1 node", () => { + const [title] = createSignal("t"); + const props = { + get title() { + return title(); + } + }; + expect(spreadNodes(() => props)).toBe(1); + expect(spreadNodes(() => props, true)).toBe(1); + expect(spreadNodes(() => ({ ...props, children: "x" }), true)).toBe(1); + }); + + test("a ref adds no node", () => { + const [title] = createSignal("t"); + const props = { + ref: () => {}, + get title() { + return title(); + } + }; + expect(spreadNodes(() => props)).toBe(1); + }); + + test("plain data children insert with no effect", () => { + expect(spreadNodes(() => ({ title: "a", children: "static" }))).toBe(1); + expect(spreadNodes(() => ({ title: "a", children: document.createElement("b") }))).toBe(1); + }); + + test("compiled mergeProps source: 1 memo for the reactive part + 1 attribute effect", () => { + const [rest] = createSignal({ "data-a": "1" }); + // `
` → spread(el, merge({class}, () => rest())) + expect(spreadNodes(() => merge({ class: "c" }, () => rest()), true)).toBe(2); + // ...and with getter children flowing through, the children insert too. + expect( + spreadNodes(() => + merge( + { + get children() { + return "c"; + } + }, + () => rest() + ) + ) + ).toBe(3); + }); + + test("array form: no memo for a function source", () => { + const [rest] = createSignal({ "data-a": "1" }); + expect(spreadNodes(() => [{ class: "c" }, () => rest()], true)).toBe(1); + expect(spreadNodes(() => [{ class: "c", children: "x" }, () => rest()])).toBe(2); + }); +}); + +describe("ref folded into the attribute effect", () => { + test("applied once on mount; not re-called when an unrelated attribute changes", () => { + const [cls, setCls] = createSignal("a"); + const refFn = vi.fn(); + const m = mount(() =>
); + expect(refFn).toHaveBeenCalledTimes(1); + expect(refFn).toHaveBeenCalledWith(m.el()); + setCls("b"); + flush(); + expect(m.el().className).toBe("b"); + expect(refFn).toHaveBeenCalledTimes(1); + m.dispose(); + }); + + test("lone reactive spread: ref applied once, survives attribute-only updates", () => { + const refFn = vi.fn(); + const [props, setProps] = createSignal({ ref: refFn, title: "a" }); + const m = mount(() =>
); + expect(refFn).toHaveBeenCalledTimes(1); + expect(refFn).toHaveBeenCalledWith(m.el()); + setProps({ ref: refFn, title: "b" }); + flush(); + expect(m.el().title).toBe("b"); + expect(refFn).toHaveBeenCalledTimes(1); + m.dispose(); + }); + + test("re-called with the element when the ref prop changes to a different function", () => { + const first = vi.fn(); + const second = vi.fn(); + const [props, setProps] = createSignal({ ref: first }); + const m = mount(() =>
); + expect(first).toHaveBeenCalledTimes(1); + setProps({ ref: second }); + flush(); + expect(first).toHaveBeenCalledTimes(1); + expect(second).toHaveBeenCalledTimes(1); + expect(second).toHaveBeenCalledWith(m.el()); + // Removing the ref and adding it back re-applies. + setProps({}); + flush(); + setProps({ ref: second }); + flush(); + expect(second).toHaveBeenCalledTimes(2); + m.dispose(); + }); + + test("array refs are applied, once", () => { + const a = vi.fn(); + const b = vi.fn(); + const [title, setTitle] = createSignal("t"); + const arr = [a, [b]]; + const m = mount(() => ( +
+ )); + expect(a).toHaveBeenCalledWith(m.el()); + expect(b).toHaveBeenCalledWith(m.el()); + setTitle("u"); + flush(); + expect(a).toHaveBeenCalledTimes(1); + expect(b).toHaveBeenCalledTimes(1); + m.dispose(); + }); + + test("ref is never written as an attribute", () => { + const m = mount(() =>
{}, id: "i" }} />); + expect(m.el().hasAttribute("ref")).toBe(false); + expect(m.el().id).toBe("i"); + m.dispose(); + }); +}); + +describe("children stay a separate effect", () => { + test("children are NOT re-created when an attribute changes", () => { + let instances = 0; + function Child() { + instances++; + return child; + } + const [cls, setCls] = createSignal("a"); + const props = { + get class() { + return cls(); + }, + get children() { + return ; + } + }; + const m = mount(() =>
); + expect(instances).toBe(1); + expect(m.el().innerHTML).toBe("child"); + setCls("b"); + flush(); + expect(m.el().className).toBe("b"); + expect(instances).toBe(1); + m.dispose(); + }); + + test("children DO update when the children getter's dependency changes", () => { + const [count, setCount] = createSignal(0); + const [cls, setCls] = createSignal("a"); + const spy = vi.fn(); + const props = { + get class() { + spy(); + return cls(); + }, + get children() { + return `n=${count()}`; + } + }; + const m = mount(() =>
); + expect(m.el().textContent).toBe("n=0"); + expect(spy).toHaveBeenCalledTimes(1); + setCount(1); + flush(); + expect(m.el().textContent).toBe("n=1"); + // The attribute effect did not re-run for a children-only change. + expect(spy).toHaveBeenCalledTimes(1); + setCls("b"); + flush(); + expect(m.el().className).toBe("b"); + expect(m.el().textContent).toBe("n=1"); + m.dispose(); + }); + + test("plain data children render; unrelated updates change nothing observable", () => { + const [title, setTitle] = createSignal("t"); + const props = { + children: "static text", + get title() { + return title(); + } + }; + const m = mount(() =>
); + expect(m.el().textContent).toBe("static text"); + const textNode = m.el().firstChild; + setTitle("u"); + flush(); + expect(m.el().title).toBe("u"); + expect(m.el().firstChild).toBe(textNode); + expect(m.el().textContent).toBe("static text"); + m.dispose(); + }); + + test("plain data children that are a function still resolve reactively", () => { + const [count, setCount] = createSignal(0); + // A function is not a JSX.Element type-wise, but insert() resolves it. + const props = { children: () => `n=${count()}` } as any; + const m = mount(() =>
); + expect(m.el().textContent).toBe("n=0"); + setCount(1); + flush(); + expect(m.el().textContent).toBe("n=1"); + m.dispose(); + }); + + test("skipChildren leaves children alone", () => { + const host = document.createElement("div"); + const dispose = createRoot(d => { + const el = document.createElement("div"); + host.appendChild(el); + spread(el, { children: "nope", id: "x" }, true); + return d; + }); + flush(); + expect(host.innerHTML).toBe('
'); + dispose(); + }); +}); + +describe("sources array", () => { + const spreadInto = ( + sources: unknown[], + skipChildren?: boolean, + skip?: (key: string) => boolean + ) => { + const host = document.createElement("div"); + let el!: HTMLElement; + const dispose = createRoot(d => { + el = document.createElement("div"); + host.appendChild(el); + spread(el, sources, skipChildren, skip); + return d; + }); + flush(); + return { el, host, dispose }; + }; + + test("later sources win; shadowed getters are never invoked; the winner is read once per run", () => { + const shadowed = vi.fn(() => "shadowed"); + const winner = vi.fn(() => "winner"); + const other = vi.fn(() => "o"); + const s = spreadInto([ + { + get title() { + return shadowed(); + }, + get "data-other"() { + return other(); + } + }, + { + get title() { + return winner(); + } + } + ]); + expect(s.el.title).toBe("winner"); + expect(s.el.getAttribute("data-other")).toBe("o"); + expect(shadowed).not.toHaveBeenCalled(); + expect(winner).toHaveBeenCalledTimes(1); + expect(other).toHaveBeenCalledTimes(1); + s.dispose(); + }); + + test("a later source's own `undefined` shadows an earlier value (Object.assign contract)", () => { + const s = spreadInto([{ title: "a" }, { title: undefined }]); + expect(s.el.hasAttribute("title")).toBe(false); + s.dispose(); + }); + + test("skip predicate: skipped keys are never read or applied", () => { + const secret = vi.fn(() => "s"); + const s = spreadInto( + [ + { + id: "keep", + get secret() { + return secret(); + } + }, + { "data-b": "b" } + ], + true, + key => key === "secret" || key === "data-b" + ); + expect(s.el.id).toBe("keep"); + expect(s.el.hasAttribute("secret")).toBe(false); + expect(s.el.hasAttribute("data-b")).toBe(false); + expect(secret).not.toHaveBeenCalled(); + s.dispose(); + }); + + test("nullish sources are skipped", () => { + const s = spreadInto([null, { id: "a" }, undefined, { title: "t" }]); + expect(s.el.id).toBe("a"); + expect(s.el.title).toBe("t"); + s.dispose(); + }); + + test("function sources are called inline and re-apply when their signal changes", () => { + const [rest, setRest] = createSignal | null>({ title: "x", "data-a": "1" }); + const calls = vi.fn(() => rest()); + // skipChildren: the children insert is its own tracking scope and resolves + // the sources itself; this pins the attribute effect's single call per run. + const s = spreadInto([{ id: "i", title: "static" }, calls], true); + expect(s.el.title).toBe("x"); + expect(s.el.getAttribute("data-a")).toBe("1"); + expect(calls).toHaveBeenCalledTimes(1); + setRest({ "data-a": "2" }); + flush(); + expect(s.el.getAttribute("data-a")).toBe("2"); + expect(s.el.title).toBe("static"); // dropped from the function source → earlier value returns + expect(calls).toHaveBeenCalledTimes(2); + setRest(null); // a nullish resolution is an empty source + flush(); + expect(s.el.hasAttribute("data-a")).toBe(false); + expect(s.el.id).toBe("i"); + s.dispose(); + }); + + test("children come from the last source that owns the key", () => { + const [n, setN] = createSignal(0); + const s = spreadInto([ + { children: "first", id: "a" }, + { + get children() { + return `second ${n()}`; + } + } + ]); + expect(s.el.textContent).toBe("second 0"); + setN(1); + flush(); + expect(s.el.textContent).toBe("second 1"); + s.dispose(); + }); + + test("ref in a sources array is applied once and diffed by identity", () => { + const refFn = vi.fn(); + const [title, setTitle] = createSignal("t"); + const s = spreadInto([{ ref: refFn }, () => ({ title: title() })]); + expect(refFn).toHaveBeenCalledTimes(1); + expect(refFn).toHaveBeenCalledWith(s.el); + setTitle("u"); + flush(); + expect(s.el.title).toBe("u"); + expect(refFn).toHaveBeenCalledTimes(1); + s.dispose(); + }); +}); + +describe("existing contracts", () => { + test("reactive lone spread still works (#3105)", () => { + const [props, setProps] = createSignal({ id: "a", title: "t" }); + const m = mount(() =>
); + expect(m.el().id).toBe("a"); + setProps({ id: "b" }); + flush(); + expect(m.el().id).toBe("b"); + expect(m.el().hasAttribute("title")).toBe(false); + m.dispose(); + }); + + test("nullish source removes attributes (#3297)", () => { + const [props, setProps] = createSignal({ id: "a" }); + const m = mount(() =>
); + expect(m.el().id).toBe("a"); + setProps(null); + flush(); + expect(m.el().hasAttribute("id")).toBe(false); + setProps({ id: "c" }); + flush(); + expect(m.el().id).toBe("c"); + m.dispose(); + }); +});