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 a8a0aa0bb3e..dba63ed16f4 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, @@ -79,14 +79,14 @@ 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'; -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'; @@ -123,12 +123,14 @@ 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. @@ -181,19 +183,23 @@ 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( - 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); @@ -216,7 +222,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 +249,7 @@ export const vnode_diff = ( advance(); } expectNoMore(); + cleanupSideBuffer(); ascend(); } } @@ -264,26 +277,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,14 +308,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) { + 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.'); + vSideBuffer = null; vSiblings = null; vSiblingsArray = null; vParent = (vNewNode || vCurrent!) as ElementVNode | VirtualVNode; @@ -327,6 +336,7 @@ export const vnode_diff = ( function ascend() { const descendVNode = stack.pop(); // boolean: descendVNode if (descendVNode) { + vSideBuffer = stack.pop(); vSiblings = stack.pop(); vSiblingsArray = stack.pop(); vNewNode = stack.pop(); @@ -343,7 +353,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, vSiblings, vSideBuffer); } stack.push(descendVNode); if (Array.isArray(children)) { @@ -510,6 +520,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 +728,16 @@ 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` - 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. - 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; - } + const currentKey = getKey(vCurrent); + if (!isSameElementName || jsxKey !== currentKey) { + 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 + deleteFromSideBuffer(elementName, jsxKey); } + // reconcile attributes const jsxAttrs = [] as ClientAttrs; @@ -792,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)) { @@ -808,7 +833,21 @@ 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; + } + clearAllEffects(container, vnode); + value = trackSignalAndAssignHost( + unwrappedSignal, + vnode, + key, + container, + NON_CONST_SUBSCRIPTION_DATA + ); } vnode.setAttr( @@ -914,18 +953,6 @@ 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. - * - * @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 @@ -946,7 +973,7 @@ export const vnode_diff = ( vSiblingsArray.push(name, vNode); } else { // we only add the elements which we did not find yet. - vSiblings.set(name + ':' + vKey, vNode); + vSiblings.set(getSideBufferKey(name, vKey), vNode); } } vNode = vNode.nextSibling as VNode | null; @@ -961,49 +988,166 @@ export const vnode_diff = ( } } } else { - const vSibling = vSiblings.get(nodeName + ':' + key); - if (vSibling) { - vNodeWithKey = vSibling as ElementVNode | VirtualVNode; - vSiblings.delete(nodeName + ':' + key); + const siblingsKey = getSideBufferKey(nodeName, key); + if (vSiblings.has(siblingsKey)) { + vNodeWithKey = vSiblings.get(siblingsKey) as ElementVNode | VirtualVNode; + vSiblings.delete(siblingsKey); } } } + + collectSideBufferSiblings(vNodeWithKey); + return vNodeWithKey; } + 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(); + vSideBuffer.set(sideBufferKey, vCurrent); + vSiblings?.delete(sideBufferKey); + } + } + + return; + } + + // 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, vNode); + vSiblings?.delete(sideBufferKey); + } + + vNode = vNode.nextSibling as VNode | null; + } + } + + 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; - 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. + deleteFromSideBuffer(null, currentKey); return; - } else if (jsxKey !== null) { - // We have a key find it - vNewNode = retrieveChildWithKey(null, jsxKey); - if (vNewNode != null) { - // We found it, move it up. - vnode_insertBefore( - journal, - vParent as VirtualVNode, - vNewNode, - vCurrent && getInsertBefore() - ); - return; - } } - // Did not find it, insert a new one. - vnode_insertBefore( - journal, + + const createNew = () => { + 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); + }; + // 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, - (vNewNode = vnode_newVirtual()), - vCurrent && getInsertBefore() + createNew, + true ); - (vNewNode as VirtualVNode).setProp(ELEMENT_KEY, jsxKey); - isDev && (vNewNode as VirtualVNode).setProp(DEBUG_TYPE, type); } function expectComponent(component: Function) { @@ -1026,21 +1170,19 @@ 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); - } else { - // We did not find the component, create it. + const createNew = () => { insertNewComponent(host, componentQRL, jsxProps); shouldRender = true; - } - host = vNewNode as VirtualVNode; + }; + 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 { + // delete the key from the side buffer if it is the same component + deleteFromSideBuffer(null, lookupKey); } if (host) { @@ -1050,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; } @@ -1089,23 +1239,21 @@ export const vnode_diff = ( const vNodeLookupKey = getKey(host); const lookupKeysAreEqual = lookupKey === vNodeLookupKey; const vNodeComponentHash = getComponentHash(host, container.$getObjectById$); + const isInlineComponent = vNodeComponentHash == null; - 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); - } else { - // We did not find the inline component, create it. - insertNewInlineComponent(); - } - host = vNewNode as VirtualVNode; - } - // inline components don't have component hash - q:renderFn prop, so it should be null - else if (vNodeComponentHash != null) { + if ((host && !isInlineComponent) || lookupKey == null) { insertNewInlineComponent(); host = vNewNode as VirtualVNode; + } else if (!lookupKeysAreEqual) { + 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 + deleteFromSideBuffer(null, lookupKey); } if (host) { @@ -1247,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) { @@ -1257,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--; } @@ -1284,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; } } @@ -1292,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 a549387739a..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', () => { @@ -431,6 +445,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 () => { @@ -839,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/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/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/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/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/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..e510e6fae74 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 unknown as SignalImpl).$wrappedSignal$ ||= new WrappedSignalImpl( + null, + getValueProp, + args, + null + )); + } else if (isStore(args[0])) { + return new WrappedSignalImpl(null, getValueProp, args, null); + } + return (args[0] as { value: T }).value; + } else { + return new WrappedSignalImpl(null, getProp, args, null); + } +}; type PropType = P extends keyof T ? T[P] 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); } 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/component.spec.tsx b/packages/qwik/src/core/tests/component.spec.tsx index 88106f0533d..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 ( @@ -2410,6 +2453,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$(() => { 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); }); }); }); 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: