From a4039d68a3e9f785c9df64f1ee8ab6d384389bd0 Mon Sep 17 00:00:00 2001 From: Varixo Date: Wed, 24 Sep 2025 18:08:17 +0200 Subject: [PATCH 1/8] perf: use replace children for truncating nodes --- packages/qwik/src/core/client/vnode-diff.ts | 4 +- packages/qwik/src/core/client/vnode.ts | 24 +++- .../qwik/src/core/tests/component.spec.tsx | 125 ++++++++++++++++++ 3 files changed, 147 insertions(+), 6 deletions(-) diff --git a/packages/qwik/src/core/client/vnode-diff.ts b/packages/qwik/src/core/client/vnode-diff.ts index a8a0aa0bb3e..269630fef88 100644 --- a/packages/qwik/src/core/client/vnode-diff.ts +++ b/packages/qwik/src/core/client/vnode-diff.ts @@ -32,7 +32,7 @@ import { dangerouslySetInnerHTML, } from '../shared/utils/markers'; import { isPromise } from '../shared/utils/promises'; -import { type ValueOrPromise } from '../shared/utils/types'; +import { isArray, type ValueOrPromise } from '../shared/utils/types'; import { getEventNameFromJsxEvent, getEventNameScopeFromJsxEvent, @@ -308,7 +308,7 @@ export const vnode_diff = ( * is an array produced by the `map` function. */ function descend(children: JSXChildren, descendVNode: boolean) { - if (children == null) { + if (children == null || (descendVNode && isArray(children) && children.length === 0)) { expectNoChildren(); return; } diff --git a/packages/qwik/src/core/client/vnode.ts b/packages/qwik/src/core/client/vnode.ts index 88df3eb4537..b7b1db26abf 100644 --- a/packages/qwik/src/core/client/vnode.ts +++ b/packages/qwik/src/core/client/vnode.ts @@ -183,8 +183,9 @@ export const enum VNodeJournalOpCode { SetText = 1, // ------ [SetAttribute, target, text] SetAttribute = 2, // - [SetAttribute, target, ...(key, values)]] HoistStyles = 3, // -- [HoistStyles, document] - Remove = 4, // ------- [Insert, target(parent), ...nodes] - Insert = 5, // ------- [Insert, target(parent), reference, ...nodes] + Remove = 4, // ------- [Remove, target(parent), ...nodes] + RemoveAll = 5, // ------- [RemoveAll, target(parent)] + Insert = 6, // ------- [Insert, target(parent), reference, ...nodes] } export type VNodeJournal = Array< @@ -968,6 +969,15 @@ export const vnode_applyJournal = (journal: VNodeJournal) => { idx++; } break; + case VNodeJournalOpCode.RemoveAll: + const removeAllParent = journal[idx++] as Element; + if (removeAllParent.replaceChildren) { + removeAllParent.replaceChildren(); + } else { + // fallback if replaceChildren is not supported + removeAllParent.textContent = ''; + } + break; case VNodeJournalOpCode.Insert: const insertParent = journal[idx++] as Element; const insertBefore = journal[idx++] as Element | Text | null; @@ -1220,8 +1230,14 @@ export const vnode_truncate = ( ) => { assertDefined(vDelete, 'Missing vDelete.'); const parent = vnode_getDomParent(vParent); - const children = vnode_getDOMChildNodes(journal, vDelete); - parent && children.length && journal.push(VNodeJournalOpCode.Remove, parent, ...children); + if (parent) { + if (vnode_isElementVNode(vParent)) { + journal.push(VNodeJournalOpCode.RemoveAll, parent); + } else { + const children = vnode_getDOMChildNodes(journal, vParent); + children.length && journal.push(VNodeJournalOpCode.Remove, parent, ...children); + } + } const vPrevious = vDelete.previousSibling; if (vPrevious) { vPrevious.nextSibling = null; diff --git a/packages/qwik/src/core/tests/component.spec.tsx b/packages/qwik/src/core/tests/component.spec.tsx index 88106f0533d..a2ce4fb69a8 100644 --- a/packages/qwik/src/core/tests/component.spec.tsx +++ b/packages/qwik/src/core/tests/component.spec.tsx @@ -2410,6 +2410,131 @@ describe.each([ await expect(document.querySelector('div')).toMatchDOM(
); }); + it('should correctly remove all children for empty array', async () => { + const Cmp = component$(() => { + const list = useSignal([1, 2, 3]); + return ( +
+ + {list.value.map((item) => ( +
{item}
+ ))} +
+ ); + }); + const { vNode, document } = await render(, { debug }); + expect(vNode).toMatchVDOM( + +
+ +
1
+
2
+
3
+
+ + ); + await trigger(document.body, 'button', 'click'); + expect(vNode).toMatchVDOM( + +
+ +
+ + ); + expect(document.querySelector('main')).toMatchDOM( +
+ +
+ ); + }); + + it('should correctly remove all children for empty array - case 2', async () => { + const Cmp = component$(() => { + const list = useSignal([1, 2, 3]); + return ( +
+ +
+ {list.value.map((item) => ( +
{item}
+ ))} +
+
+ ); + }); + const { vNode, document } = await render(, { debug }); + expect(vNode).toMatchVDOM( + +
+ +
+
1
+
2
+
3
+
+
+ + ); + await trigger(document.body, 'button', 'click'); + expect(vNode).toMatchVDOM( + +
+ +
+
+ + ); + expect(document.querySelector('main')).toMatchDOM( +
+ +
+
+ ); + }); + + it('should correctly remove all children for empty array within virtual node', async () => { + const Cmp = component$(() => { + const list = useSignal([1, 2, 3]); + return ( +
+ + <> + {list.value.map((item) => ( +
{item}
+ ))} + +
+ ); + }); + const { vNode, document } = await render(, { debug }); + expect(vNode).toMatchVDOM( + +
+ + +
1
+
2
+
3
+ +
+ + ); + await trigger(document.body, 'button', 'click'); + expect(vNode).toMatchVDOM( + +
+ + +
+ + ); + await expect(document.querySelector('main')).toMatchDOM( +
+ +
+ ); + }); + describe('regression', () => { it('#3643', async () => { const Issue3643 = component$(() => { From dddf66a16f15fd156b6bec417780a7a208188e4c Mon Sep 17 00:00:00 2001 From: Varixo Date: Sat, 27 Sep 2025 11:02:38 +0200 Subject: [PATCH 2/8] feat: faster nodes removing --- packages/qwik/src/core/client/vnode-diff.ts | 322 ++++++++++++------ packages/qwik/src/core/client/vnode-impl.ts | 8 +- packages/qwik/src/core/debug.ts | 2 + .../qwik/src/core/tests/component.spec.tsx | 43 +++ 4 files changed, 263 insertions(+), 112 deletions(-) diff --git a/packages/qwik/src/core/client/vnode-diff.ts b/packages/qwik/src/core/client/vnode-diff.ts index 269630fef88..1d26001dbd4 100644 --- a/packages/qwik/src/core/client/vnode-diff.ts +++ b/packages/qwik/src/core/client/vnode-diff.ts @@ -123,12 +123,13 @@ export const vnode_diff = ( /// and is not connected to the tree. let vNewNode: VNode | null = null; - /// When elements have keys they can be consumed out of order and therefore we can't use nextSibling. - /// In such a case this array will contain the elements after the current location. - /// The array even indices will contains keys and odd indices the vNode. - let vSiblings: Map | null = null; + /// The array even indices will contains keys and odd indices the non keyed siblings. let vSiblingsArray: Array | null = null; + /// Side buffer to store nodes that are moved out of order during key scanning. + /// This contains nodes that were found before the target key and need to be moved later. + let vSideBuffer: Map | null = null; + /// Current set of JSX children. let jsxChildren: JSXChildren[] = null!; // Current JSX child. @@ -216,7 +217,13 @@ export const vnode_diff = ( } } else if (type === Projection) { expectProjection(); - descend(jsxValue.children, true); + descend( + jsxValue.children, + true, + // special case for projection, we don't want to expect no children + // because the projection's children are not removed + false + ); } else if (type === SSRComment) { expectNoMore(); } else if (type === SSRRaw) { @@ -237,6 +244,7 @@ export const vnode_diff = ( advance(); } expectNoMore(); + cleanupSideBuffer(); ascend(); } } @@ -264,26 +272,14 @@ export const vnode_diff = ( } } - /** - * Advance the `vCurrent` to the next sibling. - * - * Normally this is just `vCurrent = vCurrent.nextSibling`. However, this gets complicated if - * `retrieveChildWithKey` was called, because then we are consuming nodes out of order and can't - * rely on `nextSibling` and instead we need to go by `vSiblings`. - */ + /** Advance the `vCurrent` to the next sibling. */ function peekNextSibling() { // If we don't have a `vNewNode`, than that means we just reconciled the current node. // So advance it. return vCurrent ? (vCurrent.nextSibling as VNode | null) : null; } - /** - * Advance the `vCurrent` to the next sibling. - * - * Normally this is just `vCurrent = vCurrent.nextSibling`. However, this gets complicated if - * `retrieveChildWithKey` was called, because then we are consuming nodes out of order and can't - * rely on `nextSibling` and instead we need to go by `vSiblings`. - */ + /** Advance the `vCurrent` to the next sibling. */ function advanceToNextSibling() { vCurrent = peekNextSibling(); } @@ -307,15 +303,22 @@ export const vnode_diff = ( * In the above example all nodes are on same level so we don't `descendVNode` even thought there * is an array produced by the `map` function. */ - function descend(children: JSXChildren, descendVNode: boolean) { - if (children == null || (descendVNode && isArray(children) && children.length === 0)) { + function descend( + children: JSXChildren, + descendVNode: boolean, + shouldExpectNoChildren: boolean = true + ) { + if ( + shouldExpectNoChildren && + (children == null || (descendVNode && isArray(children) && children.length === 0)) + ) { expectNoChildren(); return; } stackPush(children, descendVNode); if (descendVNode) { assertDefined(vCurrent || vNewNode, 'Expecting vCurrent to be defined.'); - vSiblings = null; + vSideBuffer = null; vSiblingsArray = null; vParent = (vNewNode || vCurrent!) as ElementVNode | VirtualVNode; vCurrent = vnode_getFirstChild(vParent); @@ -327,7 +330,7 @@ export const vnode_diff = ( function ascend() { const descendVNode = stack.pop(); // boolean: descendVNode if (descendVNode) { - vSiblings = stack.pop(); + vSideBuffer = stack.pop(); vSiblingsArray = stack.pop(); vNewNode = stack.pop(); vCurrent = stack.pop(); @@ -343,7 +346,7 @@ export const vnode_diff = ( function stackPush(children: JSXChildren, descendVNode: boolean) { stack.push(jsxChildren, jsxIdx, jsxCount, jsxValue); if (descendVNode) { - stack.push(vParent, vCurrent, vNewNode, vSiblingsArray, vSiblings); + stack.push(vParent, vCurrent, vNewNode, vSiblingsArray, vSideBuffer); } stack.push(descendVNode); if (Array.isArray(children)) { @@ -510,6 +513,22 @@ export const vnode_diff = ( return directGetPropsProxyProp(jsxNode, 'name') || QDefaultSlot; } + function cleanupSideBuffer() { + if (vSideBuffer) { + // Remove all nodes in the side buffer as they are no longer needed + for (const vNode of vSideBuffer.values()) { + if (vNode.flags & VNodeFlags.Deleted) { + continue; + } + cleanup(container, vNode); + vnode_remove(journal, vParent, vNode, true); + } + vSideBuffer.clear(); + vSideBuffer = null; + } + vCurrent = null; + } + function drainAsyncQueue(): ValueOrPromise { while (asyncQueue.length) { const jsxNode = asyncQueue.shift() as ValueOrPromise; @@ -702,23 +721,36 @@ export const vnode_diff = ( vCurrent && vnode_isElementVNode(vCurrent) && elementName === vnode_getElementName(vCurrent); const jsxKey: string | null = jsx.key; let needsQDispatchEventPatch = false; - if (!isSameElementName || jsxKey !== getKey(vCurrent)) { - // So we have a key and it does not match the current node. - // We need to do a forward search to find it. - // The complication is that once we start taking nodes out of order we can't use `nextSibling` + const currentKey = getKey(vCurrent); + if (!isSameElementName || jsxKey !== currentKey) { + // scan until you find the key you want. vNewNode = retrieveChildWithKey(elementName, jsxKey); - if (vNewNode === null) { - // No existing node with key exists, just create a new one. - needsQDispatchEventPatch = createNewElement(jsx, elementName); - } else { - // Existing keyed node - vnode_insertBefore(journal, vParent as ElementVNode, vNewNode, vCurrent); - // We are here, so jsx is different from the vCurrent, so now we want to point to the moved node. + // If found remove everything before and place in side buffer. + if (vNewNode) { vCurrent = vNewNode; - // We need to clean up the vNewNode, because we don't want to skip advance to next sibling (see `advance` function). vNewNode = null; + } else { + // If not found, check the side buffer + vNewNode = vSideBuffer?.get(elementName + ':' + jsxKey) || null; + if (vNewNode) { + vSideBuffer!.delete(elementName + ':' + jsxKey); + // if found insert from side-buffer + // Existing keyed node + vnode_insertBefore(journal, vParent as ElementVNode, vNewNode, vCurrent); + // We are here, so jsx is different from the vCurrent, so now we want to point to the moved node. + vCurrent = vNewNode; + // We need to clean up the vNewNode, because we don't want to skip advance to next sibling (see `advance` function). + vNewNode = null; + } else { + // if not found it is a new item create it. + needsQDispatchEventPatch = createNewElement(jsx, elementName); + } } + } else if (vSideBuffer?.has(elementName + ':' + jsxKey)) { + // delete the key from the side buffer if it is the same element + vSideBuffer.delete(elementName + ':' + jsxKey); } + // reconcile attributes const jsxAttrs = [] as ClientAttrs; @@ -918,9 +950,8 @@ export const vnode_diff = ( * This function is used to retrieve the child with the given key. If the child is not found, it * will return null. * - * After finding the first child with the given key we will create a map of all the keyed siblings - * and an array of non-keyed siblings. This is done to optimize the search for the next child with - * the specified key. + * We will also collect all the keyed siblings found before the target key and add them to the + * side buffer. This is done to optimize the search for the next child with the specified key. * * @param nodeName - The name of the node. * @param key - The key of the node. @@ -931,79 +962,127 @@ export const vnode_diff = ( key: string | null ): ElementVNode | VirtualVNode | null { let vNodeWithKey: ElementVNode | VirtualVNode | null = null; - if (vSiblings === null) { - // it is not materialized; so materialize it. - vSiblings = new Map(); - vSiblingsArray = []; - let vNode = vCurrent; - while (vNode) { - const name = vnode_isElementVNode(vNode) ? vnode_getElementName(vNode) : null; - const vKey = getKey(vNode) || getComponentHash(vNode, container.$getObjectById$); - if (vNodeWithKey === null && vKey == key && name == nodeName) { - vNodeWithKey = vNode as ElementVNode | VirtualVNode; - } else { - if (vKey === null) { - vSiblingsArray.push(name, vNode); - } else { - // we only add the elements which we did not find yet. - vSiblings.set(name + ':' + vKey, vNode); - } + + // if key is null we need to: + // - if this is the first time fill the vSiblingsArray with all siblings + // - if not then find the node we are interested in + + if (key == null && vSiblingsArray != null) { + for (let i = 0; i < vSiblingsArray.length; i += 2) { + if (vSiblingsArray[i] === nodeName) { + vNodeWithKey = vSiblingsArray![i + 1] as ElementVNode | VirtualVNode; + vSiblingsArray.splice(i, 2); + break; } - vNode = vNode.nextSibling as VNode | null; } - } else { - if (key === null) { - for (let i = 0; i < vSiblingsArray!.length; i += 2) { - if (vSiblingsArray![i] === nodeName) { - vNodeWithKey = vSiblingsArray![i + 1] as ElementVNode | VirtualVNode; - vSiblingsArray!.splice(i, 2); - break; + return vNodeWithKey; + } + + const fillSiblingsArray = vSiblingsArray == null; + let vNode = vCurrent; + let foundTarget = false; + let keyedSiblingsBeforeTarget: Array<{ + sideBufferKey: string; + vNode: VNode; + }> | null = null; + + while (vNode) { + const name = vnode_isElementVNode(vNode) ? vnode_getElementName(vNode) : null; + const vKey = getKey(vNode) || getComponentHash(vNode, container.$getObjectById$); + + if (vNodeWithKey === null && vKey == key && name == nodeName) { + vNodeWithKey = vNode as ElementVNode | VirtualVNode; + foundTarget = true; + if (keyedSiblingsBeforeTarget && keyedSiblingsBeforeTarget.length > 0) { + vSideBuffer ||= new Map(); + // Add all collected keyed siblings to side buffer now that we found the target + for (const sibling of keyedSiblingsBeforeTarget) { + vSideBuffer.set(sibling.sideBufferKey, sibling.vNode); } } + if (!fillSiblingsArray) { + break; + } } else { - const vSibling = vSiblings.get(nodeName + ':' + key); - if (vSibling) { - vNodeWithKey = vSibling as ElementVNode | VirtualVNode; - vSiblings.delete(nodeName + ':' + key); + if (vKey == null) { + if (fillSiblingsArray) { + // Unkeyed sibling - add to siblings array + vSiblingsArray ||= []; + vSiblingsArray.push(name, vNode); + } + } else { + if (!foundTarget) { + keyedSiblingsBeforeTarget ||= []; + const sideBufferKey = name ? name + ':' + vKey : vKey; + // Collect keyed sibling found before target + keyedSiblingsBeforeTarget.push({ sideBufferKey, vNode }); + } } } + + vNode = vNode.nextSibling as VNode | null; + } + + // add current to the side buffer if it is not the target + if (!foundTarget && vCurrent) { + const name = vnode_isElementVNode(vCurrent) ? vnode_getElementName(vCurrent) : null; + const vKey = getKey(vCurrent) || getComponentHash(vCurrent, container.$getObjectById$); + if (vKey != null) { + const sideBufferKey = name ? name + ':' + vKey : vKey; + vSideBuffer ||= new Map(); + vSideBuffer.set(sideBufferKey, vCurrent); + } } return vNodeWithKey; } function expectVirtual(type: VirtualType, jsxKey: string | null) { const checkKey = type === VirtualType.Fragment; - if ( + const currentKey = getKey(vCurrent); + const isSameNode = vCurrent && vnode_isVirtualVNode(vCurrent) && - getKey(vCurrent) === jsxKey && - (checkKey ? !!jsxKey : true) - ) { + currentKey === jsxKey && + (checkKey ? !!jsxKey : true); + + if (isSameNode) { // All is good. + if (currentKey && vSideBuffer?.has(currentKey)) { + vSideBuffer.delete(currentKey); + } return; - } else if (jsxKey !== null) { - // We have a key find it + } + if (jsxKey !== null) { + // Try to find the node. vNewNode = retrieveChildWithKey(null, jsxKey); - if (vNewNode != null) { - // We found it, move it up. - vnode_insertBefore( - journal, - vParent as VirtualVNode, - vNewNode, - vCurrent && getInsertBefore() - ); - return; + if (vNewNode) { + vCurrent = vNewNode; + vNewNode = null; + } else { + // If not found, check the side buffer + vNewNode = vSideBuffer?.get(jsxKey) || null; + if (vNewNode) { + // if found insert from side-buffer + vSideBuffer!.delete(jsxKey); + // Add current to the side buffer + if (vCurrent && currentKey) { + vSideBuffer?.set(currentKey, vCurrent); + } + vnode_insertBefore(journal, vParent as VirtualVNode, vNewNode, vCurrent); + } } } - // Did not find it, insert a new one. - vnode_insertBefore( - journal, - vParent as VirtualVNode, - (vNewNode = vnode_newVirtual()), - vCurrent && getInsertBefore() - ); - (vNewNode as VirtualVNode).setProp(ELEMENT_KEY, jsxKey); - isDev && (vNewNode as VirtualVNode).setProp(DEBUG_TYPE, type); + if (vNewNode === null) { + // if not found it is a new item create it. + vnode_insertBefore( + journal, + vParent as VirtualVNode, + (vNewNode = vnode_newVirtual()), + vCurrent && getInsertBefore() + ); + (vNewNode as VirtualVNode).setProp(ELEMENT_KEY, jsxKey); + isDev && (vNewNode as VirtualVNode).setProp(DEBUG_TYPE, type); + } } function expectComponent(component: Function) { @@ -1026,21 +1105,32 @@ export const vnode_diff = ( const hashesAreEqual = componentHash === vNodeComponentHash; if (!lookupKeysAreEqual) { - // See if we already have this component later on. vNewNode = retrieveChildWithKey(null, lookupKey); if (vNewNode) { - // We found the component, move it up. - vnode_insertBefore(journal, vParent as VirtualVNode, vNewNode, vCurrent); + vCurrent = vNewNode; + vNewNode = null; + host = vCurrent as VirtualVNode; } else { - // We did not find the component, create it. - insertNewComponent(host, componentQRL, jsxProps); - shouldRender = true; + vNewNode = lookupKey != null ? vSideBuffer?.get(lookupKey) || null : null; + if (vNewNode) { + vSideBuffer!.delete(lookupKey); + vnode_insertBefore(journal, vParent as VirtualVNode, vNewNode, vCurrent); + vCurrent = vNewNode; + vNewNode = null; + host = vCurrent as VirtualVNode; + } else { + insertNewComponent(host, componentQRL, jsxProps); + shouldRender = true; + host = vNewNode! as VirtualVNode; + } } - host = vNewNode as VirtualVNode; } else if (!hashesAreEqual || !jsxNode.key) { insertNewComponent(host, componentQRL, jsxProps); host = vNewNode as VirtualVNode; shouldRender = true; + } else if (vSideBuffer?.has(lookupKey)) { + // delete the key from the side buffer if it is the same component + vSideBuffer.delete(lookupKey); } if (host) { @@ -1089,23 +1179,35 @@ export const vnode_diff = ( const vNodeLookupKey = getKey(host); const lookupKeysAreEqual = lookupKey === vNodeLookupKey; const vNodeComponentHash = getComponentHash(host, container.$getObjectById$); + const isInlineComponent = vNodeComponentHash == null; - if (!lookupKeysAreEqual) { + if ((host && !isInlineComponent) || lookupKey == null) { + insertNewInlineComponent(); + host = vNewNode as VirtualVNode; + } else if (!lookupKeysAreEqual) { // See if we already have this inline component later on. vNewNode = retrieveChildWithKey(null, lookupKey); if (vNewNode) { - // We found the inline component, move it up. - vnode_insertBefore(journal, vParent as VirtualVNode, vNewNode, vCurrent); + vCurrent = vNewNode; + vNewNode = null; + host = vCurrent as VirtualVNode; } else { - // We did not find the inline component, create it. - insertNewInlineComponent(); + vNewNode = vSideBuffer?.get(lookupKey) || null; + if (vNewNode) { + vSideBuffer!.delete(lookupKey); + vnode_insertBefore(journal, vParent as VirtualVNode, vNewNode, vCurrent); + vCurrent = vNewNode; + vNewNode = null; + host = vCurrent as VirtualVNode; + } else { + // We did not find the inline component, create it. + insertNewInlineComponent(); + host = vNewNode! as VirtualVNode; + } } - host = vNewNode as VirtualVNode; - } - // inline components don't have component hash - q:renderFn prop, so it should be null - else if (vNodeComponentHash != null) { - insertNewInlineComponent(); - host = vNewNode as VirtualVNode; + } else if (vSideBuffer?.has(lookupKey)) { + // delete the key from the side buffer if it is the same component + vSideBuffer.delete(lookupKey); } if (host) { diff --git a/packages/qwik/src/core/client/vnode-impl.ts b/packages/qwik/src/core/client/vnode-impl.ts index 8c20cd1858c..b9ae72bb7eb 100644 --- a/packages/qwik/src/core/client/vnode-impl.ts +++ b/packages/qwik/src/core/client/vnode-impl.ts @@ -9,6 +9,7 @@ import { import type { ChoreArray } from './chore-array'; import { _EFFECT_BACK_REF } from '../reactive-primitives/types'; import { BackRef } from '../reactive-primitives/cleanup'; +import { isDev } from '@qwik.dev/core/build'; /** @internal */ export abstract class VNode extends BackRef { @@ -91,8 +92,11 @@ export abstract class VNode extends BackRef { } } - toString() { - return vnode_toString.call(this); + toString(): string { + if (isDev) { + return vnode_toString.call(this); + } + return String(this); } } diff --git a/packages/qwik/src/core/debug.ts b/packages/qwik/src/core/debug.ts index aad50e09d15..0bbb745b04c 100644 --- a/packages/qwik/src/core/debug.ts +++ b/packages/qwik/src/core/debug.ts @@ -50,6 +50,8 @@ export function qwikDebugToString(value: any): any { return 'Store'; } else if (isJSXNode(value)) { return jsxToString(value); + } else if (vnode_isVNode(value)) { + return '(' + value.getProp(DEBUG_TYPE, null) + ')'; } } finally { stringifyPath.pop(); diff --git a/packages/qwik/src/core/tests/component.spec.tsx b/packages/qwik/src/core/tests/component.spec.tsx index a2ce4fb69a8..e37690503cb 100644 --- a/packages/qwik/src/core/tests/component.spec.tsx +++ b/packages/qwik/src/core/tests/component.spec.tsx @@ -86,6 +86,49 @@ describe.each([ ); }); + it('should render component with key', async () => { + (globalThis as any).componentExecuted = []; + const Cmp = component$(() => { + (globalThis as any).componentExecuted.push('Cmp'); + return
; + }); + + const Parent = component$(() => { + const counter = useSignal(0); + return ( + <> + + + + ); + }); + + const { vNode, document } = await render(, { debug }); + expect((globalThis as any).componentExecuted).toEqual(['Cmp']); + expect(vNode).toMatchVDOM( + + + +
+ + + + + ); + await trigger(document.body, 'button', 'click'); + expect((globalThis as any).componentExecuted).toEqual(['Cmp', 'Cmp']); + expect(vNode).toMatchVDOM( + + + +
+ + + + + ); + }); + it('should handle null as empty string', async () => { const MyComp = component$(() => { return ( From ff61ff1780a5ffe97aa4fc2b4589ff1fcb40e5cd Mon Sep 17 00:00:00 2001 From: Varixo Date: Mon, 29 Sep 2025 15:00:27 +0200 Subject: [PATCH 3/8] refactor: move or create keyed node --- packages/qwik/src/core/client/vnode-diff.ts | 209 +++++++++++--------- 1 file changed, 114 insertions(+), 95 deletions(-) diff --git a/packages/qwik/src/core/client/vnode-diff.ts b/packages/qwik/src/core/client/vnode-diff.ts index 1d26001dbd4..63299346e04 100644 --- a/packages/qwik/src/core/client/vnode-diff.ts +++ b/packages/qwik/src/core/client/vnode-diff.ts @@ -723,32 +723,12 @@ export const vnode_diff = ( let needsQDispatchEventPatch = false; const currentKey = getKey(vCurrent); if (!isSameElementName || jsxKey !== currentKey) { - // scan until you find the key you want. - vNewNode = retrieveChildWithKey(elementName, jsxKey); - // If found remove everything before and place in side buffer. - if (vNewNode) { - vCurrent = vNewNode; - vNewNode = null; - } else { - // If not found, check the side buffer - vNewNode = vSideBuffer?.get(elementName + ':' + jsxKey) || null; - if (vNewNode) { - vSideBuffer!.delete(elementName + ':' + jsxKey); - // if found insert from side-buffer - // Existing keyed node - vnode_insertBefore(journal, vParent as ElementVNode, vNewNode, vCurrent); - // We are here, so jsx is different from the vCurrent, so now we want to point to the moved node. - vCurrent = vNewNode; - // We need to clean up the vNewNode, because we don't want to skip advance to next sibling (see `advance` function). - vNewNode = null; - } else { - // if not found it is a new item create it. - needsQDispatchEventPatch = createNewElement(jsx, elementName); - } - } - } else if (vSideBuffer?.has(elementName + ':' + jsxKey)) { + const sideBufferKey = getSideBufferKey(elementName, jsxKey); + const createNew = () => (needsQDispatchEventPatch = createNewElement(jsx, elementName)); + moveOrCreateKeyedNode(elementName, jsxKey, sideBufferKey, vParent as ElementVNode, createNew); + } else { // delete the key from the side buffer if it is the same element - vSideBuffer.delete(elementName + ':' + jsxKey); + deleteFromSideBuffer(elementName, jsxKey); } // reconcile attributes @@ -1013,7 +993,7 @@ export const vnode_diff = ( } else { if (!foundTarget) { keyedSiblingsBeforeTarget ||= []; - const sideBufferKey = name ? name + ':' + vKey : vKey; + const sideBufferKey = getSideBufferKey(name, vKey); // Collect keyed sibling found before target keyedSiblingsBeforeTarget.push({ sideBufferKey, vNode }); } @@ -1028,7 +1008,7 @@ export const vnode_diff = ( const name = vnode_isElementVNode(vCurrent) ? vnode_getElementName(vCurrent) : null; const vKey = getKey(vCurrent) || getComponentHash(vCurrent, container.$getObjectById$); if (vKey != null) { - const sideBufferKey = name ? name + ':' + vKey : vKey; + const sideBufferKey = getSideBufferKey(name, vKey); vSideBuffer ||= new Map(); vSideBuffer.set(sideBufferKey, vCurrent); } @@ -1036,6 +1016,81 @@ export const vnode_diff = ( return vNodeWithKey; } + function getSideBufferKey(nodeName: string | null, key: string): string; + function getSideBufferKey(nodeName: string | null, key: string | null): string | null; + function getSideBufferKey(nodeName: string | null, key: string | null): string | null { + if (key == null) { + return null; + } + return nodeName ? nodeName + ':' + key : key; + } + + function deleteFromSideBuffer(nodeName: string | null, key: string | null): boolean { + const sbKey = getSideBufferKey(nodeName, key); + if (sbKey && vSideBuffer?.has(sbKey)) { + vSideBuffer.delete(sbKey); + return true; + } + return false; + } + + /** + * Shared utility to resolve a keyed node by: + * + * 1. Scanning forward siblings via `retrieveChildWithKey` + * 2. Falling back to the side buffer using the provided `sideBufferKey` + * 3. Creating a new node via `createNew` when not found + * + * If a node is moved from the side buffer, it is inserted before `vCurrent` under + * `parentForInsert`. The function updates `vCurrent`/`vNewNode` accordingly and returns the value + * from `createNew` when a new node is created. + */ + function moveOrCreateKeyedNode( + nodeName: string | null, + lookupKey: string | null, + sideBufferKey: string | null, + parentForInsert: VNode, + createNew: () => any, + addCurrentToSideBufferOnSideInsert?: boolean + ): any { + // 1) Try to find the node among upcoming siblings + vNewNode = retrieveChildWithKey(nodeName, lookupKey); + if (vNewNode) { + vCurrent = vNewNode; + vNewNode = null; + return; + } + + // 2) Try side buffer + if (sideBufferKey != null) { + const buffered = vSideBuffer?.get(sideBufferKey) || null; + if (buffered) { + vSideBuffer!.delete(sideBufferKey); + if (addCurrentToSideBufferOnSideInsert && vCurrent) { + const currentKey = + getKey(vCurrent) || getComponentHash(vCurrent, container.$getObjectById$); + if (currentKey != null) { + const currentName = vnode_isElementVNode(vCurrent) + ? vnode_getElementName(vCurrent) + : null; + const currentSideKey = getSideBufferKey(currentName, currentKey); + if (currentSideKey != null) { + vSideBuffer ||= new Map(); + vSideBuffer.set(currentSideKey, vCurrent); + } + } + } + vnode_insertBefore(journal, parentForInsert as any, buffered, vCurrent); + vCurrent = buffered; + vNewNode = null; + return; + } + } + + // 3) Create new + return createNew(); + } + function expectVirtual(type: VirtualType, jsxKey: string | null) { const checkKey = type === VirtualType.Fragment; const currentKey = getKey(vCurrent); @@ -1047,33 +1102,11 @@ export const vnode_diff = ( if (isSameNode) { // All is good. - if (currentKey && vSideBuffer?.has(currentKey)) { - vSideBuffer.delete(currentKey); - } + deleteFromSideBuffer(null, currentKey); return; } - if (jsxKey !== null) { - // Try to find the node. - vNewNode = retrieveChildWithKey(null, jsxKey); - if (vNewNode) { - vCurrent = vNewNode; - vNewNode = null; - } else { - // If not found, check the side buffer - vNewNode = vSideBuffer?.get(jsxKey) || null; - if (vNewNode) { - // if found insert from side-buffer - vSideBuffer!.delete(jsxKey); - // Add current to the side buffer - if (vCurrent && currentKey) { - vSideBuffer?.set(currentKey, vCurrent); - } - vnode_insertBefore(journal, vParent as VirtualVNode, vNewNode, vCurrent); - } - } - } - if (vNewNode === null) { - // if not found it is a new item create it. + + const createNew = () => { vnode_insertBefore( journal, vParent as VirtualVNode, @@ -1082,7 +1115,20 @@ export const vnode_diff = ( ); (vNewNode as VirtualVNode).setProp(ELEMENT_KEY, jsxKey); isDev && (vNewNode as VirtualVNode).setProp(DEBUG_TYPE, type); + }; + // For fragments without a key, always create a new virtual node (ensures rerender semantics) + if (checkKey && jsxKey === null) { + createNew(); + return; } + moveOrCreateKeyedNode( + null, + jsxKey, + getSideBufferKey(null, jsxKey), + vParent as VirtualVNode, + createNew, + true + ); } function expectComponent(component: Function) { @@ -1105,32 +1151,19 @@ export const vnode_diff = ( const hashesAreEqual = componentHash === vNodeComponentHash; if (!lookupKeysAreEqual) { - vNewNode = retrieveChildWithKey(null, lookupKey); - if (vNewNode) { - vCurrent = vNewNode; - vNewNode = null; - host = vCurrent as VirtualVNode; - } else { - vNewNode = lookupKey != null ? vSideBuffer?.get(lookupKey) || null : null; - if (vNewNode) { - vSideBuffer!.delete(lookupKey); - vnode_insertBefore(journal, vParent as VirtualVNode, vNewNode, vCurrent); - vCurrent = vNewNode; - vNewNode = null; - host = vCurrent as VirtualVNode; - } else { - insertNewComponent(host, componentQRL, jsxProps); - shouldRender = true; - host = vNewNode! as VirtualVNode; - } - } + const createNew = () => { + insertNewComponent(host, componentQRL, jsxProps); + shouldRender = true; + }; + moveOrCreateKeyedNode(null, lookupKey, lookupKey, vParent as VirtualVNode, createNew); + host = (vNewNode || vCurrent) as VirtualVNode; } else if (!hashesAreEqual || !jsxNode.key) { insertNewComponent(host, componentQRL, jsxProps); host = vNewNode as VirtualVNode; shouldRender = true; - } else if (vSideBuffer?.has(lookupKey)) { + } else { // delete the key from the side buffer if it is the same component - vSideBuffer.delete(lookupKey); + deleteFromSideBuffer(null, lookupKey); } if (host) { @@ -1185,29 +1218,15 @@ export const vnode_diff = ( insertNewInlineComponent(); host = vNewNode as VirtualVNode; } else if (!lookupKeysAreEqual) { - // See if we already have this inline component later on. - vNewNode = retrieveChildWithKey(null, lookupKey); - if (vNewNode) { - vCurrent = vNewNode; - vNewNode = null; - host = vCurrent as VirtualVNode; - } else { - vNewNode = vSideBuffer?.get(lookupKey) || null; - if (vNewNode) { - vSideBuffer!.delete(lookupKey); - vnode_insertBefore(journal, vParent as VirtualVNode, vNewNode, vCurrent); - vCurrent = vNewNode; - vNewNode = null; - host = vCurrent as VirtualVNode; - } else { - // We did not find the inline component, create it. - insertNewInlineComponent(); - host = vNewNode! as VirtualVNode; - } - } - } else if (vSideBuffer?.has(lookupKey)) { + const createNew = () => { + // We did not find the inline component, create it. + insertNewInlineComponent(); + }; + moveOrCreateKeyedNode(null, lookupKey, lookupKey, vParent as VirtualVNode, createNew); + host = (vNewNode || vCurrent) as VirtualVNode; + } else { // delete the key from the side buffer if it is the same component - vSideBuffer.delete(lookupKey); + deleteFromSideBuffer(null, lookupKey); } if (host) { From 4fc7667b041f098ae93c71577d3789a7a1d6a8dc Mon Sep 17 00:00:00 2001 From: Varixo Date: Mon, 29 Sep 2025 21:26:01 +0200 Subject: [PATCH 4/8] perf: improve performance of new diff algorithm --- packages/qwik/src/core/client/vnode-diff.ts | 129 +++++++++--------- .../qwik/src/core/client/vnode-diff.unit.tsx | 24 ++++ 2 files changed, 86 insertions(+), 67 deletions(-) diff --git a/packages/qwik/src/core/client/vnode-diff.ts b/packages/qwik/src/core/client/vnode-diff.ts index 63299346e04..359997daa4a 100644 --- a/packages/qwik/src/core/client/vnode-diff.ts +++ b/packages/qwik/src/core/client/vnode-diff.ts @@ -123,6 +123,7 @@ export const vnode_diff = ( /// and is not connected to the tree. let vNewNode: VNode | null = null; + let vSiblings: Map | null = null; /// The array even indices will contains keys and odd indices the non keyed siblings. let vSiblingsArray: Array | null = null; @@ -319,6 +320,7 @@ export const vnode_diff = ( if (descendVNode) { assertDefined(vCurrent || vNewNode, 'Expecting vCurrent to be defined.'); vSideBuffer = null; + vSiblings = null; vSiblingsArray = null; vParent = (vNewNode || vCurrent!) as ElementVNode | VirtualVNode; vCurrent = vnode_getFirstChild(vParent); @@ -331,6 +333,7 @@ export const vnode_diff = ( const descendVNode = stack.pop(); // boolean: descendVNode if (descendVNode) { vSideBuffer = stack.pop(); + vSiblings = stack.pop(); vSiblingsArray = stack.pop(); vNewNode = stack.pop(); vCurrent = stack.pop(); @@ -346,7 +349,7 @@ export const vnode_diff = ( function stackPush(children: JSXChildren, descendVNode: boolean) { stack.push(jsxChildren, jsxIdx, jsxCount, jsxValue); if (descendVNode) { - stack.push(vParent, vCurrent, vNewNode, vSiblingsArray, vSideBuffer); + stack.push(vParent, vCurrent, vNewNode, vSiblingsArray, vSiblings, vSideBuffer); } stack.push(descendVNode); if (Array.isArray(children)) { @@ -926,94 +929,85 @@ export const vnode_diff = ( } } - /** - * This function is used to retrieve the child with the given key. If the child is not found, it - * will return null. - * - * We will also collect all the keyed siblings found before the target key and add them to the - * side buffer. This is done to optimize the search for the next child with the specified key. - * - * @param nodeName - The name of the node. - * @param key - The key of the node. - * @returns The child with the given key or null if not found. - */ function retrieveChildWithKey( nodeName: string | null, key: string | null ): ElementVNode | VirtualVNode | null { let vNodeWithKey: ElementVNode | VirtualVNode | null = null; - - // if key is null we need to: - // - if this is the first time fill the vSiblingsArray with all siblings - // - if not then find the node we are interested in - - if (key == null && vSiblingsArray != null) { - for (let i = 0; i < vSiblingsArray.length; i += 2) { - if (vSiblingsArray[i] === nodeName) { - vNodeWithKey = vSiblingsArray![i + 1] as ElementVNode | VirtualVNode; - vSiblingsArray.splice(i, 2); - break; + if (vSiblings === null) { + // it is not materialized; so materialize it. + vSiblings = new Map(); + vSiblingsArray = []; + let vNode = vCurrent; + while (vNode) { + const name = vnode_isElementVNode(vNode) ? vnode_getElementName(vNode) : null; + const vKey = getKey(vNode) || getComponentHash(vNode, container.$getObjectById$); + if (vNodeWithKey === null && vKey == key && name == nodeName) { + vNodeWithKey = vNode as ElementVNode | VirtualVNode; + } else { + if (vKey === null) { + vSiblingsArray.push(name, vNode); + } else { + // we only add the elements which we did not find yet. + vSiblings.set(getSideBufferKey(name, vKey), vNode); + } + } + vNode = vNode.nextSibling as VNode | null; + } + } else { + if (key === null) { + for (let i = 0; i < vSiblingsArray!.length; i += 2) { + if (vSiblingsArray![i] === nodeName) { + vNodeWithKey = vSiblingsArray![i + 1] as ElementVNode | VirtualVNode; + vSiblingsArray!.splice(i, 2); + break; + } + } + } else { + const siblingsKey = getSideBufferKey(nodeName, key); + if (vSiblings.has(siblingsKey)) { + vNodeWithKey = vSiblings.get(siblingsKey) as ElementVNode | VirtualVNode; + vSiblings.delete(siblingsKey); } } - return vNodeWithKey; } - const fillSiblingsArray = vSiblingsArray == null; - let vNode = vCurrent; - let foundTarget = false; - let keyedSiblingsBeforeTarget: Array<{ - sideBufferKey: string; - vNode: VNode; - }> | null = null; + collectSideBufferSiblings(vNodeWithKey); - while (vNode) { - const name = vnode_isElementVNode(vNode) ? vnode_getElementName(vNode) : null; - const vKey = getKey(vNode) || getComponentHash(vNode, container.$getObjectById$); + return vNodeWithKey; + } - if (vNodeWithKey === null && vKey == key && name == nodeName) { - vNodeWithKey = vNode as ElementVNode | VirtualVNode; - foundTarget = true; - if (keyedSiblingsBeforeTarget && keyedSiblingsBeforeTarget.length > 0) { + function collectSideBufferSiblings(targetNode: VNode | null): void { + if (!targetNode) { + if (vCurrent) { + const name = vnode_isElementVNode(vCurrent) ? vnode_getElementName(vCurrent) : null; + const vKey = getKey(vCurrent) || getComponentHash(vCurrent, container.$getObjectById$); + if (vKey != null) { + const sideBufferKey = getSideBufferKey(name, vKey); vSideBuffer ||= new Map(); - // Add all collected keyed siblings to side buffer now that we found the target - for (const sibling of keyedSiblingsBeforeTarget) { - vSideBuffer.set(sibling.sideBufferKey, sibling.vNode); - } - } - if (!fillSiblingsArray) { - break; - } - } else { - if (vKey == null) { - if (fillSiblingsArray) { - // Unkeyed sibling - add to siblings array - vSiblingsArray ||= []; - vSiblingsArray.push(name, vNode); - } - } else { - if (!foundTarget) { - keyedSiblingsBeforeTarget ||= []; - const sideBufferKey = getSideBufferKey(name, vKey); - // Collect keyed sibling found before target - keyedSiblingsBeforeTarget.push({ sideBufferKey, vNode }); - } + vSideBuffer.set(sideBufferKey, vCurrent); + vSiblings?.delete(sideBufferKey); } } - vNode = vNode.nextSibling as VNode | null; + return; } - // add current to the side buffer if it is not the target - if (!foundTarget && vCurrent) { - const name = vnode_isElementVNode(vCurrent) ? vnode_getElementName(vCurrent) : null; - const vKey = getKey(vCurrent) || getComponentHash(vCurrent, container.$getObjectById$); + // Walk from vCurrent up to the target node and collect all keyed siblings + let vNode = vCurrent; + while (vNode && vNode !== targetNode) { + const name = vnode_isElementVNode(vNode) ? vnode_getElementName(vNode) : null; + const vKey = getKey(vNode) || getComponentHash(vNode, container.$getObjectById$); + if (vKey != null) { const sideBufferKey = getSideBufferKey(name, vKey); vSideBuffer ||= new Map(); - vSideBuffer.set(sideBufferKey, vCurrent); + vSideBuffer.set(sideBufferKey, vNode); + vSiblings?.delete(sideBufferKey); } + + vNode = vNode.nextSibling as VNode | null; } - return vNodeWithKey; } function getSideBufferKey(nodeName: string | null, key: string): string; @@ -1055,6 +1049,7 @@ export const vnode_diff = ( ): any { // 1) Try to find the node among upcoming siblings vNewNode = retrieveChildWithKey(nodeName, lookupKey); + if (vNewNode) { vCurrent = vNewNode; vNewNode = null; diff --git a/packages/qwik/src/core/client/vnode-diff.unit.tsx b/packages/qwik/src/core/client/vnode-diff.unit.tsx index a549387739a..79f239bc212 100644 --- a/packages/qwik/src/core/client/vnode-diff.unit.tsx +++ b/packages/qwik/src/core/client/vnode-diff.unit.tsx @@ -431,6 +431,30 @@ describe('vNode-diff', () => { expect(b1).toBe(selectB1()); expect(b2).toBe(selectB2()); }); + + it('should remove or add keyed nodes', () => { + const { vNode, vParent, container } = vnode_fromJSX( + _jsxSorted( + 'test', + {}, + null, + [_jsxSorted('b', {}, null, '1', 0, '1'), _jsxSorted('b', {}, null, '2', 0, null)], + 0, + 'KA_6' + ) + ); + const test = _jsxSorted( + 'test', + {}, + null, + [_jsxSorted('b', {}, null, '2', 0, null), _jsxSorted('b', {}, null, '2', 0, '2')], + 0, + 'KA_6' + ); + vnode_diff(container, test, vParent, null); + vnode_applyJournal(container.$journal$); + expect(vNode).toMatchVDOM(test); + }); }); describe('fragments', () => { it('should not rerender signal wrapper fragment', async () => { From 8049e6cc04705ce00d220b7df1c873af8edd0cdd Mon Sep 17 00:00:00 2001 From: Varixo Date: Tue, 30 Sep 2025 14:44:50 +0200 Subject: [PATCH 5/8] perf: remove unnecessary effects cleanups --- packages/qwik/src/core/client/vnode-diff.ts | 3 --- packages/qwik/src/core/shared/component-execution.ts | 2 +- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/packages/qwik/src/core/client/vnode-diff.ts b/packages/qwik/src/core/client/vnode-diff.ts index 359997daa4a..d9b212ca14b 100644 --- a/packages/qwik/src/core/client/vnode-diff.ts +++ b/packages/qwik/src/core/client/vnode-diff.ts @@ -183,9 +183,6 @@ export const vnode_diff = ( if (Array.isArray(jsxValue)) { descend(jsxValue, false); } else if (isSignal(jsxValue)) { - if (vCurrent) { - clearAllEffects(container, vCurrent); - } expectVirtual(VirtualType.WrappedSignal, null); descend( trackSignalAndAssignHost( diff --git a/packages/qwik/src/core/shared/component-execution.ts b/packages/qwik/src/core/shared/component-execution.ts index ade736e308e..d803792976a 100644 --- a/packages/qwik/src/core/shared/component-execution.ts +++ b/packages/qwik/src/core/shared/component-execution.ts @@ -101,7 +101,7 @@ export const executeComponent = ( container.setHostProp(renderHost, USE_ON_LOCAL_SEQ_IDX, null); } - if (vnode_isVNode(renderHost)) { + if (retryCount > 0 && vnode_isVNode(renderHost)) { clearAllEffects(container, renderHost); } From 906698a1f1320ab29131ac78005526e52ca5244d Mon Sep 17 00:00:00 2001 From: Varixo Date: Wed, 1 Oct 2025 20:42:27 +0200 Subject: [PATCH 6/8] perf: create wrapped signal only once for vnode diff # Conflicts: # packages/qwik/src/core/reactive-primitives/internal-api.ts # Conflicts: # packages/qwik/src/core/tests/render-api.spec.tsx --- packages/qwik/src/core/client/vnode-diff.ts | 29 ++++++++++++------- .../reactive-primitives/impl/signal-impl.ts | 3 +- .../impl/wrapped-signal-impl.ts | 10 ++++++- .../core/reactive-primitives/internal-api.ts | 24 ++++++++++++--- packages/qwik/src/core/ssr/ssr-render-jsx.ts | 5 +++- .../qwik/src/core/tests/render-api.spec.tsx | 4 +-- 6 files changed, 55 insertions(+), 20 deletions(-) diff --git a/packages/qwik/src/core/client/vnode-diff.ts b/packages/qwik/src/core/client/vnode-diff.ts index d9b212ca14b..2289bf5ffad 100644 --- a/packages/qwik/src/core/client/vnode-diff.ts +++ b/packages/qwik/src/core/client/vnode-diff.ts @@ -83,10 +83,10 @@ import { clearAllEffects } from '../reactive-primitives/cleanup'; import { serializeAttribute } from '../shared/utils/styles'; import { QError, qError } from '../shared/error/error'; import { getFileLocationFromJsx } from '../shared/utils/jsx-filename'; -import { EffectProperty } from '../reactive-primitives/types'; +import { EffectProperty, EffectSubscriptionProp } from '../reactive-primitives/types'; import { SubscriptionData } from '../reactive-primitives/subscription-data'; import { WrappedSignalImpl } from '../reactive-primitives/impl/wrapped-signal-impl'; -import { _CONST_PROPS, _VAR_PROPS } from '../internal'; +import { _CONST_PROPS, _EFFECT_BACK_REF, _VAR_PROPS } from '../internal'; import { isSyncQrl } from '../shared/qrl/qrl-utils'; import type { ElementVNode, TextVNode, VirtualVNode, VNode } from './vnode-impl'; @@ -184,15 +184,22 @@ export const vnode_diff = ( descend(jsxValue, false); } else if (isSignal(jsxValue)) { expectVirtual(VirtualType.WrappedSignal, null); - descend( - trackSignalAndAssignHost( - jsxValue as Signal, - (vNewNode || vCurrent)!, - EffectProperty.VNODE, - container - ), - true - ); + const unwrappedSignal = + jsxValue instanceof WrappedSignalImpl ? jsxValue.$unwrapIfSignal$() : jsxValue; + const currentSignal = vCurrent?.[_EFFECT_BACK_REF]?.get(EffectProperty.VNODE)?.[ + EffectSubscriptionProp.CONSUMER + ]; + if (currentSignal !== unwrappedSignal) { + descend( + trackSignalAndAssignHost( + unwrappedSignal, + (vNewNode || vCurrent)!, + EffectProperty.VNODE, + container + ), + true + ); + } } else if (isPromise(jsxValue)) { expectVirtual(VirtualType.Awaited, null); asyncQueue.push(jsxValue, vNewNode || vCurrent); diff --git a/packages/qwik/src/core/reactive-primitives/impl/signal-impl.ts b/packages/qwik/src/core/reactive-primitives/impl/signal-impl.ts index d64c8fbd3cb..6845433c9d1 100644 --- a/packages/qwik/src/core/reactive-primitives/impl/signal-impl.ts +++ b/packages/qwik/src/core/reactive-primitives/impl/signal-impl.ts @@ -13,6 +13,7 @@ import { import type { Signal } from '../signal.public'; import { SignalFlags, type EffectSubscription } from '../types'; import { ChoreType } from '../../shared/util-chore-type'; +import type { WrappedSignalImpl } from './wrapped-signal-impl'; const DEBUG = false; // eslint-disable-next-line no-console @@ -23,8 +24,8 @@ export class SignalImpl implements Signal { /** Store a list of effects which are dependent on this signal. */ $effects$: null | Set = null; - $container$: Container | null = null; + $wrappedSignal$: WrappedSignalImpl | null = null; constructor(container: Container | null, value: T) { this.$container$ = container; diff --git a/packages/qwik/src/core/reactive-primitives/impl/wrapped-signal-impl.ts b/packages/qwik/src/core/reactive-primitives/impl/wrapped-signal-impl.ts index 4810bc624ab..fd744510585 100644 --- a/packages/qwik/src/core/reactive-primitives/impl/wrapped-signal-impl.ts +++ b/packages/qwik/src/core/reactive-primitives/impl/wrapped-signal-impl.ts @@ -4,6 +4,7 @@ import type { Container, HostElement } from '../../shared/types'; import { ChoreType } from '../../shared/util-chore-type'; import { trackSignal } from '../../use/use-core'; import type { BackRef } from '../cleanup'; +import { getValueProp } from '../internal-api'; import type { AllSignalFlags, EffectSubscription } from '../types'; import { _EFFECT_BACK_REF, @@ -12,7 +13,7 @@ import { SignalFlags, WrappedSignalFlags, } from '../types'; -import { scheduleEffects } from '../utils'; +import { isSignal, scheduleEffects } from '../utils'; import { SignalImpl } from './signal-impl'; export class WrappedSignalImpl extends SignalImpl implements BackRef { @@ -106,6 +107,13 @@ export class WrappedSignalImpl extends SignalImpl implements BackRef { this.$untrackedValue$ = untrackedValue; } } + + $unwrapIfSignal$(): SignalImpl | WrappedSignalImpl { + return this.$func$ === getValueProp && isSignal(this.$args$[0]) + ? (this.$args$[0] as SignalImpl) + : this; + } + // Make this signal read-only set value(_: any) { throw qError(QError.wrappedReadOnly); diff --git a/packages/qwik/src/core/reactive-primitives/internal-api.ts b/packages/qwik/src/core/reactive-primitives/internal-api.ts index 0e91dc679ed..57a5f0643e0 100644 --- a/packages/qwik/src/core/reactive-primitives/internal-api.ts +++ b/packages/qwik/src/core/reactive-primitives/internal-api.ts @@ -2,18 +2,34 @@ import { _CONST_PROPS, _IMMUTABLE } from '../shared/utils/constants'; import { assertEqual } from '../shared/error/assert'; import { isObject } from '../shared/utils/types'; import { isSignal, type Signal } from './signal.public'; -import { getStoreTarget } from './impl/store'; +import { getStoreTarget, isStore } from './impl/store'; import { isPropsProxy } from '../shared/jsx/jsx-runtime'; import { WrappedSignalFlags } from './types'; import { WrappedSignalImpl } from './impl/wrapped-signal-impl'; import { AsyncComputedSignalImpl } from './impl/async-computed-signal-impl'; +import type { SignalImpl } from './impl/signal-impl'; // Keep these properties named like this so they're the same as from wrapSignal -const getValueProp = (p0: { value: T }) => p0.value; +export const getValueProp = (p0: { value: T }) => p0.value; const getProp = (p0: T, p1: P) => p0[p1]; -const getWrapped = (args: [T, (keyof T | undefined)?]) => - new WrappedSignalImpl(null, args.length === 1 ? getValueProp : getProp, args, null); +const getWrapped = (args: [T, (keyof T | undefined)?]) => { + if (args.length === 1) { + if (isSignal(args[0])) { + return ((args[0] as SignalImpl).$wrappedSignal$ ||= new WrappedSignalImpl( + null, + getValueProp, + args, + null + )); + } else if (isStore(args[0])) { + return new WrappedSignalImpl(null, getValueProp, args, null); + } + return args[0].value; + } else { + return new WrappedSignalImpl(null, getProp, args, null); + } +}; type PropType = P extends keyof T ? T[P] diff --git a/packages/qwik/src/core/ssr/ssr-render-jsx.ts b/packages/qwik/src/core/ssr/ssr-render-jsx.ts index bcc49bc3c28..5503c3448e3 100644 --- a/packages/qwik/src/core/ssr/ssr-render-jsx.ts +++ b/packages/qwik/src/core/ssr/ssr-render-jsx.ts @@ -129,8 +129,11 @@ function processJSXNode( } else if (isSignal(value)) { ssr.openFragment(isDev ? [DEBUG_TYPE, VirtualType.WrappedSignal] : EMPTY_ARRAY); const signalNode = ssr.getOrCreateLastNode(); + const unwrappedSignal = value instanceof WrappedSignalImpl ? value.$unwrapIfSignal$() : value; enqueue(ssr.closeFragment); - enqueue(() => trackSignalAndAssignHost(value, signalNode, EffectProperty.VNODE, ssr)); + enqueue(() => + trackSignalAndAssignHost(unwrappedSignal, signalNode, EffectProperty.VNODE, ssr) + ); enqueue(MaybeAsyncSignal); } else if (isPromise(value)) { ssr.openFragment(isDev ? [DEBUG_TYPE, VirtualType.Awaited] : EMPTY_ARRAY); diff --git a/packages/qwik/src/core/tests/render-api.spec.tsx b/packages/qwik/src/core/tests/render-api.spec.tsx index e6ffe3ad3af..fc645ddbc4a 100644 --- a/packages/qwik/src/core/tests/render-api.spec.tsx +++ b/packages/qwik/src/core/tests/render-api.spec.tsx @@ -800,7 +800,7 @@ describe('render api', () => { }} > {v}} /> - {sig.value} + {sig.value + 'test'} ); }); @@ -1012,7 +1012,7 @@ describe('render api', () => { streaming, }); // This can change when the size of the output changes - expect(stream.write).toHaveBeenCalledTimes(6); + expect(stream.write).toHaveBeenCalledTimes(5); }); }); }); From b71f35426e7166ab3f4df6140eb07b873f6eec39 Mon Sep 17 00:00:00 2001 From: Varixo Date: Thu, 2 Oct 2025 08:47:33 +0200 Subject: [PATCH 7/8] perf: create wrapped signal only once for attributes diff --- packages/qwik/src/core/client/vnode-diff.ts | 15 ++++++++++++++- .../src/core/reactive-primitives/internal-api.ts | 4 ++-- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/packages/qwik/src/core/client/vnode-diff.ts b/packages/qwik/src/core/client/vnode-diff.ts index 2289bf5ffad..59402098074 100644 --- a/packages/qwik/src/core/client/vnode-diff.ts +++ b/packages/qwik/src/core/client/vnode-diff.ts @@ -827,7 +827,20 @@ export const vnode_diff = ( } if (isSignal(value)) { - value = trackSignalAndAssignHost(value, vnode, key, container, NON_CONST_SUBSCRIPTION_DATA); + const unwrappedSignal = + value instanceof WrappedSignalImpl ? value.$unwrapIfSignal$() : value; + const currentSignal = + vnode?.[_EFFECT_BACK_REF]?.get(key)?.[EffectSubscriptionProp.CONSUMER]; + if (currentSignal === unwrappedSignal) { + return; + } + value = trackSignalAndAssignHost( + unwrappedSignal, + vnode, + key, + container, + NON_CONST_SUBSCRIPTION_DATA + ); } vnode.setAttr( diff --git a/packages/qwik/src/core/reactive-primitives/internal-api.ts b/packages/qwik/src/core/reactive-primitives/internal-api.ts index 57a5f0643e0..e510e6fae74 100644 --- a/packages/qwik/src/core/reactive-primitives/internal-api.ts +++ b/packages/qwik/src/core/reactive-primitives/internal-api.ts @@ -16,7 +16,7 @@ const getProp = (p0: T, p1: P) => p0[p1]; const getWrapped = (args: [T, (keyof T | undefined)?]) => { if (args.length === 1) { if (isSignal(args[0])) { - return ((args[0] as SignalImpl).$wrappedSignal$ ||= new WrappedSignalImpl( + return ((args[0] as unknown as SignalImpl).$wrappedSignal$ ||= new WrappedSignalImpl( null, getValueProp, args, @@ -25,7 +25,7 @@ const getWrapped = (args: [T, (keyof T | undefined)?]) => { } else if (isStore(args[0])) { return new WrappedSignalImpl(null, getValueProp, args, null); } - return args[0].value; + return (args[0] as { value: T }).value; } else { return new WrappedSignalImpl(null, getProp, args, null); } From 74c570c22436cbd5417ae4036f309ccdb3d72dc4 Mon Sep 17 00:00:00 2001 From: Varixo Date: Tue, 7 Oct 2025 19:12:27 +0200 Subject: [PATCH 8/8] fix: memory leak for reactive attributes --- .changeset/tasty-glasses-bet.md | 5 + packages/qwik/src/core/client/vnode-diff.ts | 40 +- .../qwik/src/core/client/vnode-diff.unit.tsx | 372 +++++++++++++++++- .../src/core/reactive-primitives/cleanup.ts | 32 +- pnpm-lock.yaml | 2 +- 5 files changed, 424 insertions(+), 27 deletions(-) create mode 100644 .changeset/tasty-glasses-bet.md diff --git a/.changeset/tasty-glasses-bet.md b/.changeset/tasty-glasses-bet.md new file mode 100644 index 00000000000..bbc666e2c8e --- /dev/null +++ b/.changeset/tasty-glasses-bet.md @@ -0,0 +1,5 @@ +--- +'@qwik.dev/core': patch +--- + +fix: memory leak for reactive attributes diff --git a/packages/qwik/src/core/client/vnode-diff.ts b/packages/qwik/src/core/client/vnode-diff.ts index 59402098074..dba63ed16f4 100644 --- a/packages/qwik/src/core/client/vnode-diff.ts +++ b/packages/qwik/src/core/client/vnode-diff.ts @@ -79,7 +79,7 @@ import type { Signal } from '../reactive-primitives/signal.public'; import { executeComponent } from '../shared/component-execution'; import { isSlotProp } from '../shared/utils/prop'; import { escapeHTML } from '../shared/utils/character-escaping'; -import { clearAllEffects } from '../reactive-primitives/cleanup'; +import { clearAllEffects, clearEffectSubscription } from '../reactive-primitives/cleanup'; import { serializeAttribute } from '../shared/utils/styles'; import { QError, qError } from '../shared/error/error'; import { getFileLocationFromJsx } from '../shared/utils/jsx-filename'; @@ -811,6 +811,12 @@ export const vnode_diff = ( return; } + // Clear current effect subscription if it exists + const currentEffect = vnode?.[_EFFECT_BACK_REF]?.get(key); + if (currentEffect) { + clearEffectSubscription(container, currentEffect); + } + if (key === 'ref') { const element = vnode.element; if (isSignal(value)) { @@ -834,6 +840,7 @@ export const vnode_diff = ( if (currentSignal === unwrappedSignal) { return; } + clearAllEffects(container, vnode); value = trackSignalAndAssignHost( unwrappedSignal, vnode, @@ -1185,7 +1192,15 @@ export const vnode_diff = ( ); let propsAreDifferent = false; if (!shouldRender) { - propsAreDifferent = propsDiffer(jsxProps, vNodeProps); + propsAreDifferent = + propsDiffer( + (jsxProps as PropsProxy)[_CONST_PROPS], + (vNodeProps as PropsProxy)?.[_CONST_PROPS] + ) || + propsDiffer( + (jsxProps as PropsProxy)[_VAR_PROPS], + (vNodeProps as PropsProxy)?.[_VAR_PROPS] + ); shouldRender = shouldRender || propsAreDifferent; } @@ -1380,7 +1395,10 @@ function getComponentHash(vNode: VNode | null, getObject: (id: string) => any): */ function Projection() {} -function propsDiffer(src: Record, dst: Record): boolean { +function propsDiffer( + src: Record | null | undefined, + dst: Record | null | undefined +): boolean { const srcEmpty = isPropsEmpty(src); const dstEmpty = isPropsEmpty(dst); if (srcEmpty && dstEmpty) { @@ -1390,22 +1408,22 @@ function propsDiffer(src: Record, dst: Record): boolea return true; } - const srcKeys = Object.keys(src); - const dstKeys = Object.keys(dst); + const srcKeys = Object.keys(src!); + const dstKeys = Object.keys(dst!); let srcLen = srcKeys.length; let dstLen = dstKeys.length; - if ('children' in src) { + if ('children' in src!) { srcLen--; } - if (QBackRefs in src) { + if (QBackRefs in src!) { srcLen--; } - if ('children' in dst) { + if ('children' in dst!) { dstLen--; } - if (QBackRefs in dst) { + if (QBackRefs in dst!) { dstLen--; } @@ -1417,7 +1435,7 @@ function propsDiffer(src: Record, dst: Record): boolea if (key === 'children' || key === QBackRefs) { continue; } - if (!Object.prototype.hasOwnProperty.call(dst, key) || src[key] !== dst[key]) { + if (!Object.prototype.hasOwnProperty.call(dst, key) || src![key] !== dst![key]) { return true; } } @@ -1425,7 +1443,7 @@ function propsDiffer(src: Record, dst: Record): boolea return false; } -function isPropsEmpty(props: Record): boolean { +function isPropsEmpty(props: Record | null | undefined): boolean { if (!props) { return true; } diff --git a/packages/qwik/src/core/client/vnode-diff.unit.tsx b/packages/qwik/src/core/client/vnode-diff.unit.tsx index 79f239bc212..8ba6c2be742 100644 --- a/packages/qwik/src/core/client/vnode-diff.unit.tsx +++ b/packages/qwik/src/core/client/vnode-diff.unit.tsx @@ -1,4 +1,4 @@ -import { Fragment, _fnSignal, _jsxSorted } from '@qwik.dev/core'; +import { Fragment, _fnSignal, _jsxSorted, component$ } from '@qwik.dev/core'; import { vnode_fromJSX } from '@qwik.dev/core/testing'; import { describe, expect, it } from 'vitest'; import { vnode_applyJournal, vnode_getFirstChild, vnode_getNode } from './vnode'; @@ -8,6 +8,20 @@ import { createSignal } from '../reactive-primitives/signal-api'; import { QError, qError } from '../shared/error/error'; import { VNodeFlags } from './types'; import type { VirtualVNode } from './vnode-impl'; +import type { SignalImpl } from '../reactive-primitives/impl/signal-impl'; +import type { WrappedSignalImpl } from '../reactive-primitives/impl/wrapped-signal-impl'; +import { + _hasStoreEffects, + getStoreHandler, + getOrCreateStore, +} from '../reactive-primitives/impl/store'; +import { StoreFlags } from '../reactive-primitives/types'; +import type { Scheduler } from '../shared/scheduler'; +import { ChoreType } from '../shared/util-chore-type'; + +async function waitForDrain(scheduler: Scheduler) { + await scheduler(ChoreType.WAIT_FOR_QUEUE).$returnValue$; +} describe('vNode-diff', () => { it('should find no difference', () => { @@ -863,6 +877,362 @@ describe('vNode-diff', () => { const firstChild = vnode_getFirstChild(vParent); expect(firstChild).toMatchVDOM(); }); + + describe('cleanup', () => { + it('should cleanup effects when signal attribute is replaced with non-signal', () => { + const { vParent, container } = vnode_fromJSX(_jsxSorted('span', {}, null, [], 0, null)); + const signal = createSignal('initial') as SignalImpl; + const test1 = _jsxSorted('span', { class: signal }, null, [], 0, null); + vnode_diff(container, test1, vParent, null); + vnode_applyJournal(container.$journal$); + const firstChild = vnode_getFirstChild(vParent); + expect(firstChild).toMatchVDOM(); + + // Verify signal has effects registered + expect(signal.$effects$).toBeDefined(); + expect(signal.$effects$!.size).toBeGreaterThan(0); + + // Replace signal with regular string value + const test2 = _jsxSorted('span', { class: 'static' }, null, [], 0, null); + container.$journal$ = []; + vnode_diff(container, test2, vParent, null); + vnode_applyJournal(container.$journal$); + expect(firstChild).toMatchVDOM(); + + // Verify effects have been cleaned up + expect(signal.$effects$!.size).toBe(0); + }); + + it('should cleanup effects when signal attribute is replaced with another signal', () => { + const { vParent, container } = vnode_fromJSX(_jsxSorted('span', {}, null, [], 0, null)); + const signal1 = createSignal('first') as SignalImpl; + const test1 = _jsxSorted('span', { class: signal1 }, null, [], 0, null); + vnode_diff(container, test1, vParent, null); + vnode_applyJournal(container.$journal$); + const firstChild = vnode_getFirstChild(vParent); + expect(firstChild).toMatchVDOM(); + + // Verify first signal has effects registered + expect(signal1.$effects$).toBeDefined(); + expect(signal1.$effects$!.size).toBeGreaterThan(0); + + // Replace with another signal + const signal2 = createSignal('second') as SignalImpl; + const test2 = _jsxSorted('span', { class: signal2 }, null, [], 0, null); + container.$journal$ = []; + vnode_diff(container, test2, vParent, null); + vnode_applyJournal(container.$journal$); + expect(firstChild).toMatchVDOM(); + + // Verify first signal's effects have been cleaned up + expect(signal1.$effects$!.size).toBe(0); + // Verify second signal has effects registered + expect(signal2.$effects$).toBeDefined(); + expect(signal2.$effects$!.size).toBeGreaterThan(0); + }); + + it('should cleanup effects when attribute is removed', () => { + const { vParent, container } = vnode_fromJSX(_jsxSorted('span', {}, null, [], 0, null)); + const signal = createSignal('test') as SignalImpl; + const test1 = _jsxSorted('span', { class: signal }, null, [], 0, null); + vnode_diff(container, test1, vParent, null); + vnode_applyJournal(container.$journal$); + const firstChild = vnode_getFirstChild(vParent); + expect(firstChild).toMatchVDOM(); + + // Verify signal has effects registered + expect(signal.$effects$).toBeDefined(); + expect(signal.$effects$!.size).toBeGreaterThan(0); + + // Remove the attribute entirely + const test2 = _jsxSorted('span', {}, null, [], 0, null); + container.$journal$ = []; + vnode_diff(container, test2, vParent, null); + vnode_applyJournal(container.$journal$); + expect(firstChild).toMatchVDOM(); + + // Verify effects have been cleaned up + expect(signal.$effects$!.size).toBe(0); + }); + }); + + describe('wrapped cleanup', () => { + it('should cleanup effects when wrapped signal attribute is replaced with non-signal', () => { + const { vParent, container } = vnode_fromJSX(_jsxSorted('span', {}, null, [], 0, null)); + const inner = createSignal('initial') as SignalImpl; + const wrapped1 = _fnSignal( + () => inner.value, + [], + '() => inner.value' + ) as WrappedSignalImpl; + const test1 = _jsxSorted('span', { class: wrapped1 }, null, [], 0, null); + vnode_diff(container, test1, vParent, null); + vnode_applyJournal(container.$journal$); + const firstChild = vnode_getFirstChild(vParent); + expect(firstChild).toMatchVDOM(); + + // Verify inner signal has effects registered via wrapped signal + expect(inner.$effects$).toBeDefined(); + expect(inner.$effects$!.size).toBeGreaterThan(0); + // Verify wrapped signal has effects registered + expect(wrapped1.$effects$).toBeDefined(); + expect(wrapped1.$effects$!.size).toBeGreaterThan(0); + + // Replace wrapped signal with regular string value + const test2 = _jsxSorted('span', { class: 'static' }, null, [], 0, null); + container.$journal$ = []; + vnode_diff(container, test2, vParent, null); + vnode_applyJournal(container.$journal$); + expect(firstChild).toMatchVDOM(); + + // Verify inner signal's effects have been cleaned up + expect(inner.$effects$!.size).toBe(0); + // Verify wrapped signal's effects have been cleaned up + expect(wrapped1.$effects$!.size).toBe(0); + }); + + it('should cleanup effects when wrapped signal attribute is replaced with another wrapped signal', () => { + const { vParent, container } = vnode_fromJSX(_jsxSorted('span', {}, null, [], 0, null)); + const inner1 = createSignal('first') as SignalImpl; + const wrapped1 = _fnSignal( + () => inner1.value, + [], + '() => inner1.value' + ) as WrappedSignalImpl; + const test1 = _jsxSorted('span', { class: wrapped1 }, null, [], 0, null); + vnode_diff(container, test1, vParent, null); + vnode_applyJournal(container.$journal$); + const firstChild = vnode_getFirstChild(vParent); + expect(firstChild).toMatchVDOM(); + + // Verify first inner signal has effects registered + expect(inner1.$effects$).toBeDefined(); + expect(inner1.$effects$!.size).toBeGreaterThan(0); + // Verify first wrapped signal has effects registered + expect(wrapped1.$effects$).toBeDefined(); + expect(wrapped1.$effects$!.size).toBeGreaterThan(0); + + // Replace with another wrapped signal using a different inner signal + const inner2 = createSignal('second') as SignalImpl; + const wrapped2 = _fnSignal( + () => inner2.value, + [], + '() => inner2.value' + ) as WrappedSignalImpl; + const test2 = _jsxSorted('span', { class: wrapped2 }, null, [], 0, null); + container.$journal$ = []; + vnode_diff(container, test2, vParent, null); + vnode_applyJournal(container.$journal$); + expect(firstChild).toMatchVDOM(); + + // Verify first inner signal's effects have been cleaned up + expect(inner1.$effects$!.size).toBe(0); + // Verify first wrapped signal's effects have been cleaned up + expect(wrapped1.$effects$!.size).toBe(0); + // Verify second inner signal has effects registered + expect(inner2.$effects$).toBeDefined(); + expect(inner2.$effects$!.size).toBeGreaterThan(0); + // Verify second wrapped signal has effects registered + expect(wrapped2.$effects$).toBeDefined(); + expect(wrapped2.$effects$!.size).toBeGreaterThan(0); + }); + + it('should cleanup effects when wrapped signal attribute is removed', () => { + const { vParent, container } = vnode_fromJSX(_jsxSorted('span', {}, null, [], 0, null)); + const inner = createSignal('test') as SignalImpl; + const wrapped = _fnSignal( + () => inner.value, + [], + '() => inner.value' + ) as WrappedSignalImpl; + const test1 = _jsxSorted('span', { class: wrapped }, null, [], 0, null); + vnode_diff(container, test1, vParent, null); + vnode_applyJournal(container.$journal$); + const firstChild = vnode_getFirstChild(vParent); + expect(firstChild).toMatchVDOM(); + + // Verify inner signal has effects registered + expect(inner.$effects$).toBeDefined(); + expect(inner.$effects$!.size).toBeGreaterThan(0); + // Verify wrapped signal has effects registered + expect(wrapped.$effects$).toBeDefined(); + expect(wrapped.$effects$!.size).toBeGreaterThan(0); + + // Remove the attribute entirely + const test2 = _jsxSorted('span', {}, null, [], 0, null); + container.$journal$ = []; + vnode_diff(container, test2, vParent, null); + vnode_applyJournal(container.$journal$); + expect(firstChild).toMatchVDOM(); + + // Verify effects have been cleaned up + expect(inner.$effects$!.size).toBe(0); + expect(wrapped.$effects$!.size).toBe(0); + }); + }); + + describe('store wrapped cleanup', () => { + it('should cleanup effects when store wrapped attribute is replaced with non-signal', () => { + const { vParent, container } = vnode_fromJSX(_jsxSorted('span', {}, null, [], 0, null)); + const store = getOrCreateStore({ cls: 'initial' }, StoreFlags.RECURSIVE, container); + const wrapped1 = _fnSignal( + () => store.cls, + [], + '() => store.cls' + ) as WrappedSignalImpl; + const test1 = _jsxSorted('span', { class: wrapped1 }, null, [], 0, null); + vnode_diff(container, test1, vParent, null); + vnode_applyJournal(container.$journal$); + const firstChild = vnode_getFirstChild(vParent); + expect(firstChild).toMatchVDOM(); + + // Verify store has effects registered for the property via wrapped signal + expect(_hasStoreEffects(store as any, 'cls')).toBe(true); + // Verify wrapped signal has effects registered + expect(wrapped1.$effects$).toBeDefined(); + expect(wrapped1.$effects$!.size).toBeGreaterThan(0); + + // Replace wrapped signal with regular string value + const test2 = _jsxSorted('span', { class: 'static' }, null, [], 0, null); + container.$journal$ = []; + vnode_diff(container, test2, vParent, null); + vnode_applyJournal(container.$journal$); + expect(firstChild).toMatchVDOM(); + + // Verify store's effects have been cleaned up + expect(_hasStoreEffects(store, 'cls')).toBe(false); + // Verify wrapped signal's effects have been cleaned up + expect(wrapped1.$effects$!.size).toBe(0); + }); + + it('should cleanup effects when store wrapped attribute is replaced with another store wrapped attribute', () => { + const { vParent, container } = vnode_fromJSX(_jsxSorted('span', {}, null, [], 0, null)); + const store1 = getOrCreateStore({ cls: 'first' }, StoreFlags.RECURSIVE, container); + const wrapped1 = _fnSignal( + () => store1.cls, + [], + '() => store1.cls' + ) as WrappedSignalImpl; + const test1 = _jsxSorted('span', { class: wrapped1 }, null, [], 0, null); + vnode_diff(container, test1, vParent, null); + vnode_applyJournal(container.$journal$); + const firstChild = vnode_getFirstChild(vParent); + expect(firstChild).toMatchVDOM(); + + // Verify first store has effects registered and wrapped has effects + expect(_hasStoreEffects(store1 as any, 'cls')).toBe(true); + expect(wrapped1.$effects$).toBeDefined(); + expect(wrapped1.$effects$!.size).toBeGreaterThan(0); + + // Replace with another wrapped signal using a different store + const store2 = getOrCreateStore({ cls: 'second' }, StoreFlags.RECURSIVE, container); + const wrapped2 = _fnSignal( + () => store2.cls, + [], + '() => store2.cls' + ) as WrappedSignalImpl; + const test2 = _jsxSorted('span', { class: wrapped2 }, null, [], 0, null); + container.$journal$ = []; + vnode_diff(container, test2, vParent, null); + vnode_applyJournal(container.$journal$); + expect(firstChild).toMatchVDOM(); + + // Verify first store/ wrapped effects have been cleaned up + expect(_hasStoreEffects(store1 as any, 'cls')).toBe(false); + expect(wrapped1.$effects$!.size).toBe(0); + // Verify second store/ wrapped have effects registered + expect(_hasStoreEffects(store2 as any, 'cls')).toBe(true); + expect(wrapped2.$effects$).toBeDefined(); + expect(wrapped2.$effects$!.size).toBeGreaterThan(0); + }); + + it('should cleanup effects when store wrapped attribute is removed', () => { + const { vParent, container } = vnode_fromJSX(_jsxSorted('span', {}, null, [], 0, null)); + const store = getOrCreateStore({ cls: 'test' }, StoreFlags.RECURSIVE, container); + const wrapped = _fnSignal( + () => store.cls, + [], + '() => store.cls' + ) as WrappedSignalImpl; + const test1 = _jsxSorted('span', { class: wrapped }, null, [], 0, null); + vnode_diff(container, test1, vParent, null); + vnode_applyJournal(container.$journal$); + const firstChild = vnode_getFirstChild(vParent); + expect(firstChild).toMatchVDOM(); + + // Verify store and wrapped have effects registered + expect(_hasStoreEffects(store as any, 'cls')).toBe(true); + expect(wrapped.$effects$).toBeDefined(); + expect(wrapped.$effects$!.size).toBeGreaterThan(0); + + // Remove the attribute entirely + const test2 = _jsxSorted('span', {}, null, [], 0, null); + container.$journal$ = []; + vnode_diff(container, test2, vParent, null); + vnode_applyJournal(container.$journal$); + expect(firstChild).toMatchVDOM(); + + // Verify effects have been cleaned up + expect(_hasStoreEffects(store as any, 'cls')).toBe(false); + expect(wrapped.$effects$!.size).toBe(0); + }); + }); + + describe('component props differ subscriptions', () => { + it('should not create duplicate subscription for wrapped signal prop when props differ', async () => { + const { vParent, container } = vnode_fromJSX(_jsxSorted('div', {}, null, [], 0, null)); + + const inner = createSignal('cls') as SignalImpl; + const wrapped = _fnSignal( + () => inner.value, + [], + '() => inner.value' + ) as WrappedSignalImpl; + + const Child = component$((props: any) => { + return ; + }); + + // Initial render with wrapped signal prop + const test1 = _jsxSorted( + Child as unknown as any, + null, + { cls: wrapped, foo: 1 } as any, + null, + 3, + null + ) as any; + vnode_diff(container, test1, vParent, null); + await waitForDrain(container.$scheduler$); + vnode_applyJournal(container.$journal$); + + // Ensure one subscription exists for both wrapped and inner + expect(wrapped.$effects$).not.toBeNull(); + const wrappedEffectsAfterFirst = wrapped.$effects$!.size; + expect(wrappedEffectsAfterFirst).toBeGreaterThan(0); + expect(inner.$effects$).not.toBeNull(); + const innerEffectsAfterFirst = inner.$effects$!.size; + expect(innerEffectsAfterFirst).toBeGreaterThan(0); + + // Update unrelated prop to trigger propsDiffer without touching wrapped signal prop + const test2 = _jsxSorted( + Child as unknown as any, + null, + { cls: wrapped, foo: 2 } as any, + null, + 3, + null + ) as any; + container.$journal$ = []; + vnode_diff(container, test2, vParent, null); + await waitForDrain(container.$scheduler$); + vnode_applyJournal(container.$journal$); + + // The number of effects should not increase (no duplicate subscriptions) + expect(wrapped.$effects$!.size).toBe(wrappedEffectsAfterFirst); + expect(inner.$effects$!.size).toBe(innerEffectsAfterFirst); + }); + }); }); describe('edge cases and complex scenarios', () => { diff --git a/packages/qwik/src/core/reactive-primitives/cleanup.ts b/packages/qwik/src/core/reactive-primitives/cleanup.ts index c9e56393f79..ec1280c5200 100644 --- a/packages/qwik/src/core/reactive-primitives/cleanup.ts +++ b/packages/qwik/src/core/reactive-primitives/cleanup.ts @@ -26,20 +26,24 @@ export function clearAllEffects(container: Container, consumer: Consumer): void return; } for (const [, effect] of effects) { - const backRefs = effect[EffectSubscriptionProp.BACK_REF]; - if (!backRefs) { - return; - } - for (const producer of backRefs) { - if (producer instanceof SignalImpl) { - clearSignal(container, producer, effect); - } else if (producer instanceof AsyncComputedSignalImpl) { - clearAsyncComputedSignal(producer, effect); - } else if (container.$storeProxyMap$.has(producer)) { - const target = container.$storeProxyMap$.get(producer)!; - const storeHandler = getStoreHandler(target)!; - clearStore(storeHandler, effect); - } + clearEffectSubscription(container, effect); + } +} + +export function clearEffectSubscription(container: Container, effect: EffectSubscription) { + const backRefs = effect[EffectSubscriptionProp.BACK_REF]; + if (!backRefs) { + return; + } + for (const producer of backRefs) { + if (producer instanceof SignalImpl) { + clearSignal(container, producer, effect); + } else if (producer instanceof AsyncComputedSignalImpl) { + clearAsyncComputedSignal(producer, effect); + } else if (container.$storeProxyMap$.has(producer)) { + const target = container.$storeProxyMap$.get(producer)!; + const storeHandler = getStoreHandler(target)!; + clearStore(storeHandler, effect); } } } diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 973c953d29b..361586a59ff 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -7677,7 +7677,7 @@ packages: puppeteer@22.13.1: resolution: {integrity: sha512-PwXLDQK5u83Fm5A7TGMq+9BR7iHDJ8a3h21PSsh/E6VfhxiKYkU7+tvGZNSCap6k3pCNDd9oNteVBEctcBalmQ==} engines: {node: '>=18'} - deprecated: < 24.15.0 is no longer supported + deprecated: < 24.10.2 is no longer supported hasBin: true pure-rand@6.1.0: