diff --git a/.changeset/lucky-moons-remount.md b/.changeset/lucky-moons-remount.md new file mode 100644 index 00000000000..167992954c1 --- /dev/null +++ b/.changeset/lucky-moons-remount.md @@ -0,0 +1,5 @@ +--- +'@qwik.dev/core': patch +--- + +fix: a component without its own dom element no longer rebuilds its children on re-render and keeps its `useOn` document and window events diff --git a/packages/qwik/src/core/shared/component-execution.ts b/packages/qwik/src/core/shared/component-execution.ts index f7dc1088835..de26e4ad0e9 100644 --- a/packages/qwik/src/core/shared/component-execution.ts +++ b/packages/qwik/src/core/shared/component-execution.ts @@ -7,8 +7,7 @@ import { invokeApply, newRenderInvokeContext, type RenderInvokeContext } from '. import { type EventQRL, type UseOnEvent, type UseOnMap } from '../use/use-on'; import { isQwikComponent, type OnRenderFn } from './component.public'; import { assertDefined } from './error/assert'; -import { Fragment, type Props } from './jsx/jsx-runtime'; -import { _jsxSorted } from './jsx/jsx-internal'; +import { type Props } from './jsx/jsx-runtime'; import { JSXNodeImpl, isJSXNode } from './jsx/jsx-node'; import type { JSXNodeInternal, JSXOutput } from './jsx/types/jsx-node'; import type { KnownEventNames } from './jsx/types/jsx-qwik-events'; @@ -23,9 +22,10 @@ import { OnRenderProp, USE_ON_LOCAL, USE_ON_LOCAL_SEQ_IDX, + USE_ON_PLACEHOLDER_KEY, } from './utils/markers'; import { MAX_RETRY_ON_PROMISE_COUNT, isPromise, maybeThen, safeCall } from './utils/promises'; -import { isArray, isPrimitiveOrNullUndefined, type ValueOrPromise } from './utils/types'; +import { isArray, type ValueOrPromise } from './utils/types'; import { getSubscriber } from '../reactive-primitives/subscriber'; import { EffectProperty } from '../reactive-primitives/types'; import { EventNameHtmlScope, getEventDataFromHtmlAttribute } from './utils/event-names'; @@ -340,44 +340,15 @@ function findFirstElementNode(jsx: JSXOutput): ValueOrPromise): JSXOutput { - // For regular JSX nodes, we can append the placeholder to its children. - if (isJSXNode(jsx)) { - // Inline components may ignore children, while component$ children become projections. - if (jsx.type !== Fragment) { - return _jsxSorted(Fragment, null, null, [jsx, placeholder], 0, null); - } - - if (jsx.children == null) { - jsx.children = placeholder; - } else if (isArray(jsx.children)) { - jsx.children.push(placeholder); - } else { - jsx.children = [jsx.children, placeholder]; - } - return jsx; - } - - // For primitives, we can't add children, so we wrap them in a fragment. - if (isPrimitiveOrNullUndefined(jsx)) { - return _jsxSorted(Fragment, null, null, [jsx, placeholder], 0, null); - } - - // For an array of nodes, we inject the placeholder into the first element. - if (isArray(jsx) && jsx.length > 0) { - injectPlaceholderElement(jsx[0], placeholder); - return jsx; - } - - // For anything else we do nothing. - return jsx; + return isArray(jsx) ? [...jsx, placeholder] : [jsx, placeholder]; } /** @returns An empty + Count: {'123'}! + ); @@ -372,8 +373,9 @@ describe.each([ expect(vNode).toMatchVDOM( - Count: {'124'}! + Count: {'124'}! + ); }); @@ -412,28 +414,24 @@ describe.each([ const { vNode, container } = await render(, { debug }); expect(vNode).toMatchVDOM( - - - - Count: {'123'}! - - - + Count: {'123'}! + ); @@ -634,8 +633,9 @@ describe.each([ expect(vNode).toMatchVDOM( - Count: {'124'}! + Count: {'124'}! + ); }); @@ -1059,11 +1059,9 @@ describe.each([ - -
test
- - +
test
 + ); @@ -1089,12 +1087,10 @@ describe.each([ expect(vNode).toMatchVDOM( - - -
test
- - - + +
test
+ + ); diff --git a/packages/qwik/src/core/tests/use-visible-task.spec.tsx b/packages/qwik/src/core/tests/use-visible-task.spec.tsx index 3901ca23653..95816dccb1b 100644 --- a/packages/qwik/src/core/tests/use-visible-task.spec.tsx +++ b/packages/qwik/src/core/tests/use-visible-task.spec.tsx @@ -32,6 +32,7 @@ import { ErrorProvider } from '../../testing/rendering.unit-util'; import { vnode_getProp } from '../client/vnode-utils'; import { USE_ON_LOCAL } from '../shared/utils/markers'; import { Task, TaskFlags } from '../use/use-task'; +import type { JSXNodeInternal, JSXOutput } from '../shared/jsx/types/jsx-node'; const debug = false; //true; Error.stackTraceLimit = 100; @@ -273,12 +274,35 @@ describe.each([ {'run'} - + ); }); + it('should trigger in empty components returning a list of components', async () => { + const Child = component$(() => child); + const Cmp = component$(() => { + const signal = useSignal('empty'); + useVisibleTask$(() => { + signal.value = 'run'; + }); + return [, , signal.value]; + }); + const { document } = await render(, { debug }); + if (render === ssrRenderToDom) { + await trigger(document.body, 'script', 'd:qinit'); + } + // the placeholder is the anchor for the visible task, so it has to be rendered + const anchors = document.querySelectorAll('script[q-d\\:qinit]'); + expect(anchors.length).toBe(1); + if (render === ssrRenderToDom) { + // the attribute must hold the qrl itself (chunk#symbol) - an empty one loads nothing + expect(anchors[0].getAttribute('q-d:qinit')).toContain('#'); + } + expect(document.body.textContent).toContain('run'); + }); + it('should trigger in empty components array', async () => { const Cmp = component$(() => { const signal = useSignal('empty'); @@ -295,11 +319,11 @@ describe.each([ {'run'} - {'run'} + ); }); @@ -318,9 +342,8 @@ describe.each([ } expect(vNode).toMatchVDOM( - - - + + ); }); @@ -340,10 +363,8 @@ describe.each([ expect((globalThis as any).log).toEqual(['task']); expect(vNode).toMatchVDOM( - - {''} - - + {''} + ); }); @@ -363,10 +384,8 @@ describe.each([ expect((globalThis as any).log).toEqual(['task']); expect(vNode).toMatchVDOM( - - {''} - - + {''} + ); }); @@ -904,6 +923,170 @@ describe.each([ }); describe('regression', () => { + it('should not re-create the root child component on re-render', async () => { + (globalThis as any).childRenderCounter = 0; + (globalThis as any).childInstanceCounter = 0; + const Child = component$<{ text: string }>((props) => { + (globalThis as any).childRenderCounter++; + useTask$(() => { + // no `track()`, so this runs once per instance + (globalThis as any).childInstanceCounter++; + }); + return {props.text}; + }); + + const Cmp = component$(() => { + const isLoaded = useSignal(false); + useVisibleTask$(() => { + isLoaded.value = true; + }); + const text = `v${isLoaded.value}`; + return ; + }); + + const { document } = await render(, { debug }); + const spanBeforeRerender = document.querySelector('span'); + if (render === ssrRenderToDom) { + await trigger(document.body, 'script', 'd:qinit'); + } + + expect(document.querySelector('span')?.textContent).toBe('vtrue'); + expect((globalThis as any).childRenderCounter).toBe(1); + expect((globalThis as any).childInstanceCounter).toBe(1); + // the same dom node, not a rebuilt one (only observable after a resume, because a client only + // render has already re-rendered by the time it returns) + expect(document.querySelector('span')).toBe(spanBeforeRerender); + (globalThis as any).childRenderCounter = undefined; + (globalThis as any).childInstanceCounter = undefined; + }); + + it('should not re-create the root child when output changes from child to array', async () => { + (globalThis as any).childRenderCounter = 0; + (globalThis as any).childInstanceCounter = 0; + const Child = component$<{ text: string }>((props) => { + (globalThis as any).childRenderCounter++; + useTask$(() => { + (globalThis as any).childInstanceCounter++; + }); + return {props.text}; + }); + + const Cmp = component$(() => { + const isLoaded = useSignal(false); + useVisibleTask$(() => { + isLoaded.value = true; + }); + const text = `v${isLoaded.value}`; + const child = ; + return isLoaded.value ? [child] : child; + }); + + const { document } = await render(, { debug }); + const spanBeforeRerender = document.querySelector('span'); + if (render === ssrRenderToDom) { + await trigger(document.body, 'script', 'd:qinit'); + } + + expect(document.querySelector('span')?.textContent).toBe('vtrue'); + expect((globalThis as any).childRenderCounter).toBe(1); + expect((globalThis as any).childInstanceCounter).toBe(1); + expect(document.querySelector('span')).toBe(spanBeforeRerender); + (globalThis as any).childRenderCounter = undefined; + (globalThis as any).childInstanceCounter = undefined; + }); + + it('should not modify a jsx node which the component returns', async () => { + const shared = (shared) as JSXNodeInternal; + + const WithTask = component$(() => { + useVisibleTask$(() => {}); + return shared as JSXOutput; + }); + // returns the same node, but registers nothing of its own + const NoHooks = component$(() => shared as JSXOutput); + + const Cmp = component$(() => ( +
+ + +
+ )); + + const { vNode, document } = await render(, { debug }); + if (render === ssrRenderToDom) { + await trigger(document.body, 'script', 'd:qinit'); + } + + // the returned node stays untouched + expect(shared.children).toBe('shared'); + expect(document.querySelectorAll('script[q-d\\:qinit]').length).toBe(1); + expect(vNode).toMatchVDOM( + +
+ + {'shared'} + + + + {'shared'} + +
+ + ); + }); + + it('should keep every headless component instance when the list is reordered', async () => { + (globalThis as any).childRenderCounter = 0; + (globalThis as any).childInstanceCounter = 0; + const Child = component$<{ label: string }>((props) => { + (globalThis as any).childRenderCounter++; + useTask$(() => { + (globalThis as any).childInstanceCounter++; + }); + return {props.label}; + }); + + const Headless = component$<{ name: string; index: number }>((props) => { + useVisibleTask$(() => {}); + return ; + }); + + const Cmp = component$(() => { + const names = useSignal(['a', 'b', 'c']); + return ( +
+ + {names.value.map((name, index) => ( + + ))} +
+ ); + }); + + const { document } = await render(, { debug }); + if (render === ssrRenderToDom) { + await trigger(document.body, 'script', 'd:qinit'); + } + const spansBeforeReorder = Array.from(document.querySelectorAll('span')); + expect(spansBeforeReorder.map((span) => span.textContent)).toEqual(['a0', 'b1', 'c2']); + expect((globalThis as any).childRenderCounter).toBe(3); + expect((globalThis as any).childInstanceCounter).toBe(3); + + await trigger(document.body, 'button', 'click'); + + const spansAfterReorder = Array.from(document.querySelectorAll('span')); + expect(spansAfterReorder.map((span) => span.textContent)).toEqual(['c0', 'b1', 'a2']); + expect(spansAfterReorder[0]).toBe(spansBeforeReorder[2]); + expect(spansAfterReorder[1]).toBe(spansBeforeReorder[1]); + expect(spansAfterReorder[2]).toBe(spansBeforeReorder[0]); + expect((globalThis as any).childRenderCounter).toBe(3); + expect((globalThis as any).childInstanceCounter).toBe(3); + // one placeholder per instance, they do not share or steal each other's + expect(document.querySelectorAll('script[q-d\\:qinit]').length).toBe(3); + (globalThis as any).childRenderCounter = undefined; + (globalThis as any).childInstanceCounter = undefined; + }); + it('should not double-register events on component re-render', async () => { const Cmp = component$(() => { const count = useSignal(0);