fix(core): stop rebuilding a headless component's output on every re-render

[wermetal]

What is it?

• Bug

Description

Fixes #8980

A component which renders no DOM element of its own gets a placeholder <script>, so that a document or window useOn event - useVisibleTask$ among them - has something to sit on. injectPlaceholderElement put that placeholder into the output in four ways which were wrong:

1. Everything below the component was destroyed and rebuilt on every re-render. The output was wrapped together with the placeholder in a fragment without a key, and a new wrapper was built on every render. expectVirtual never matches an unkeyed fragment against the existing tree, so a new vnode was inserted, descend() turned on creation mode, and every component below lost its state and its DOM. After SSR the whole subtree was rebuilt on resume.

   Nothing a user writes reaches that rule, because the optimizer keys every JSX node in dev and in
   production - this wrapper was the only fragment in core without a key.

2. A component returning a list of nodes lost the placeholder, so its useVisibleTask$ never
   ran after a resume:

   if (isArray(jsx) && jsx.length > 0) {
       injectPlaceholderElement(jsx[0], placeholder);   // the returned wrapper was discarded
       return jsx;
   }

   The recursive call returns a new fragment and the caller ignored it. It only worked when jsx[0]
   happened to be a fragment, because that branch modified the node in place.

3. A component returning a signal or a promise lost it too - both fell through to
   // For anything else we do nothing.

4. The placeholder was pushed into the node the component returned. For a component returning a
   node it does not rebuild - hoisted or shared JSX - that node kept the <script> for good, and
   every other component returning the same node then rendered a <script> carrying the first
   component's task.

The change

Whatever the component returns is now placed, together with the placeholder, in a single fragment
carrying a stable key, and the returned node is never written to:

• an array becomes the children of that fragment,
• a fragment is rebuilt with one more child, keeping its own key when it has one,
• everything else - a component, an element, text, a signal, a promise, nothing at all - is
  wrapped in it.

Because the shape of the output no longer decides the shape of the tree, a component which returns a
single node on one render and a list on the next keeps its children instead of rebuilding them.

Behaviour changes

• A component returning a list is rendered inside that fragment, one level deeper than before, with
  the placeholder as its last child. qinit handlers run in document order, so a list whose first
  item was a fragment used to run the parent's task before the tasks of the components inside the
  list; it now runs after them, which is what a component with a single root has always done. The
  VDOM assertion in should trigger in empty components array is updated for this.
• A fragment returned without a key by such a component is given the wrapper key, so it is matched
  across re-renders like every other output. The same component without a document or window
  useOn event still gets a new fragment on every render, because unkeyed fragments are
  deliberately never matched by expectVirtual.

Not changed

The rule in expectVirtual itself. Relaxing it would let a fragment reuse the vnode of a
component standing in the same position, with that component's QRL, props and tasks still
attached; doing it safely needs the diff to know which kind of virtual node it is looking at, and a
vnode does not carry that in a production build. Worth noting that before eb493deb3 ("fix: signal
wrapper should not rerender") every unkeyed virtual node was re-created, and that commit made an
exception for the other kinds and left fragments behind - so the rule looks like leftover behaviour
rather than a decision. It still affects JSX built at runtime, or shipped by a package compiled with
another toolchain, since that JSX has no keys.

error-boundary.ts also builds its client side fallback in an unkeyed fragment, so a boundary
showing its fallback rebuilds that subtree on every re-render. Left alone here, because reset may
depend on it.

Tests

Added to use-visible-task.spec.tsx, each running under both domRender and ssrRenderToDom, all
of them failing on main:

• should not re-create the root child component on re-render - the child records the prop it was
  born with, so vtrue|vfalse proves the instance survived; it also asserts the <span> is the same
  DOM node after a resume
• should not re-create the root child when output changes from child to array
• should trigger in empty components returning a list of components - the anchor is rendered, and
  under SSR it carries the QRL rather than an empty attribute
• should not modify a jsx node which the component returns - the returned node is untouched, and
  the component which registered nothing renders no <script>
• should keep every instance of a headless component when there are many of them - twenty of them
  on one page, keyed and unkeyed, none mixed up by the shared wrapper key
• should trigger in empty components array - updated for the new placeholder position

Checklist

• My code follows the developer guidelines of this project
• I performed a self-review of my own code
• I added a changeset with pnpm change
• I made corresponding changes to the Qwik docs
• I added new tests to cover the fix / functionality

[changeset-bot]

🦋 Changeset detected

Latest commit: 8436357

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages

Name	Type
@qwik.dev/core	Patch
@qwik.dev/devtools	Patch
eslint-plugin-qwik	Patch
@qwik.dev/react	Patch
@qwik.dev/router	Patch
@qwik.dev/utils	Patch
create-qwik	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[pkg-pr-new]

Open in StackBlitz

@qwik.dev/core

npm i https://pkg.pr.new/QwikDev/qwik/@qwik.dev/core@8981

@qwik.dev/router

npm i https://pkg.pr.new/QwikDev/qwik/@qwik.dev/router@8981

@qwik.dev/utils

npm i https://pkg.pr.new/QwikDev/qwik/@qwik.dev/utils@8981

eslint-plugin-qwik

npm i https://pkg.pr.new/QwikDev/qwik/eslint-plugin-qwik@8981

create-qwik

npm i https://pkg.pr.new/QwikDev/qwik/create-qwik@8981

@qwik.dev/optimizer

npm i https://pkg.pr.new/QwikDev/qwik/@qwik.dev/optimizer@8981

@qwik.dev/devtools

npm i https://pkg.pr.new/QwikDev/qwik/@qwik.dev/devtools@8981

commit: 8436357

[Varixo]

Hello, thank you for the PR. I added some comments.
However additional wrapper is a bigger problem than it looks. This will have big serialization impact.

how about something easier. We could create no fragment wrapper at all.

1. lets do injectPlaceholderElement returning only [jsx, placeholder] for any case
2. createPlaceholderScriptNode will get :useOn as a key instead of null

didnt check it but it should work (of course the use-on and yours tests will need changes and removing fragment wrapper from expectations)

[wermetal]

@Varixo thanks, your idea works! I switched to it.

injectPlaceholderElement now returns [jsx, placeholder] (spread for arrays), the placeholder gets the :useOn key, and the constant moved to markers.ts.

You were right about the size. SSR output for 50 headless components with useOnDocument + useVisibleTask$:

version	HTML	vnode data
main	27377	4198
my old fix	27727	4548
your idea	27077	3648

All other comments are fixed. Please review.

[Varixo]

LGTM, thanks for your work!