refactor: useOn event handling

Author: Varixo

.changeset/tiny-cows-pick.md

@@ -0,0 +1,5 @@
+---
+'@qwik.dev/core': patch
+---
+
+fix: using useOn and useVisibleTask$ in component with primitive value only

packages/qwik/src/core/shared/component-execution.ts

@@ -131,77 +131,81 @@ export const executeComponent = (
 };
 
 /**
- * Stores the JSX output of the last execution of the component.
+ * Adds `useOn` events to the JSX output.
  *
- * Component can execute multiple times because:
- *
- * - Component can have multiple tasks
- * - Tasks can track signals
- * - Task A can change signal which causes Task B to rerun.
- *
- * So when executing a component we only care about its last JSX Output.
+ * @param jsx The JSX output to modify.
+ * @param useOnEvents The `useOn` events to add.
+ * @returns The modified JSX output.
  */
-
 function addUseOnEvents(
   jsx: JSXOutput,
   useOnEvents: UseOnMap
 ): ValueOrPromise<JSXNodeInternal<string> | null | JSXOutput> {
-  const jsxElement = findFirstStringJSX(jsx);
+  const jsxElement = findFirstElementNode(jsx);
   let jsxResult = jsx;
+  const qVisibleEvent = 'onQvisible$';
   return maybeThen(jsxElement, (jsxElement) => {
-    let isInvisibleComponent = false;
-    if (!jsxElement) {
-      /**
-       * We did not find any jsx node with a string tag. This means that we should append:
-       *
-       * ```html
-       * <script type="placeholder" hidden on-document:qinit="..."></script>
-       * ```
-       *
-       * This is needed because use on events should have a node to attach them to.
-       */
-      isInvisibleComponent = true;
-    }
+    // headless components are components that don't render a real DOM element
+    const isHeadless = !jsxElement;
+    // placeholder element is a <script> element that is used to add events to the document or window
+    let placeholderElement: JSXNodeInternal<string> | null = null;
     for (const key in useOnEvents) {
       if (Object.prototype.hasOwnProperty.call(useOnEvents, key)) {
-        if (isInvisibleComponent) {
-          if (key === 'onQvisible$') {
-            const [jsxElement, jsx] = addScriptNodeForInvisibleComponents(jsxResult);
-            jsxResult = jsx;
-            if (jsxElement) {
-              addUseOnEvent(jsxElement, 'document:onQinit$', useOnEvents[key]);
-            }
-          } else if (
+        let targetElement = jsxElement;
+        let eventKey = key;
+
+        if (isHeadless) {
+          // if the component is headless, we need to add the event to the placeholder element
+          if (
+            key === qVisibleEvent ||
             key.startsWith(EventNameJSXScope.document) ||
             key.startsWith(EventNameJSXScope.window)
           ) {
-            const [jsxElement, jsx] = addScriptNodeForInvisibleComponents(jsxResult);
-            jsxResult = jsx;
-            if (jsxElement) {
-              addUseOnEvent(jsxElement, key, useOnEvents[key]);
+            if (!placeholderElement) {
+              const [createdElement, newJsx] = injectPlaceholderElement(jsxResult);
+              jsxResult = newJsx;
+              placeholderElement = createdElement;
+            }
+            targetElement = placeholderElement;
+          } else {
+            if (isDev) {
+              logWarn(
+                'You are trying to add an event "' +
+                  key +
+                  '" using `useOn` hook, ' +
+                  'but a node to which you can add an event is not found. ' +
+                  'Please make sure that the component has a valid element node. '
+              );
             }
-          } else if (isDev) {
+            continue;
+          }
+        }
+        if (targetElement) {
+          if (targetElement.type === 'script' && key === qVisibleEvent) {
+            eventKey = 'document:onQinit$';
             logWarn(
               'You are trying to add an event "' +
                 key +
-                '" using `useOn` hook, ' +
+                '" using `useVisibleTask$` hook, ' +
                 'but a node to which you can add an event is not found. ' +
-                'Please make sure that the component has a valid element node. '
+                'Using document:onQinit$ instead.'
             );
           }
-        } else if (jsxElement) {
-          if (jsxElement.type === 'script' && key === 'onQvisible$') {
-            addUseOnEvent(jsxElement, 'document:onQinit$', useOnEvents[key]);
-          } else {
-            addUseOnEvent(jsxElement, key, useOnEvents[key]);
-          }
+          addUseOnEvent(targetElement, eventKey, useOnEvents[key]);
         }
       }
     }
     return jsxResult;
   });
 }
 
+/**
+ * Adds an event to the JSX element.
+ *
+ * @param jsxElement The JSX element to add the event to.
+ * @param key The event name.
+ * @param value The event value.
+ */
 function addUseOnEvent(
   jsxElement: JSXNodeInternal,
   key: string,
@@ -221,7 +225,13 @@ function addUseOnEvent(
   props[key] = propValue;
 }
 
-function findFirstStringJSX(jsx: JSXOutput): ValueOrPromise<JSXNodeInternal<string> | null> {
+/**
+ * Finds the first element node in the JSX output.
+ *
+ * @param jsx The JSX output to search.
+ * @returns The first element node or null if no element node is found.
+ */
+function findFirstElementNode(jsx: JSXOutput): ValueOrPromise<JSXNodeInternal<string> | null> {
   const queue: any[] = [jsx];
   while (queue.length) {
     const jsx = queue.shift();
@@ -234,45 +244,67 @@ function findFirstStringJSX(jsx: JSXOutput): ValueOrPromise<JSXNodeInternal<stri
       queue.push(...jsx);
     } else if (isPromise(jsx)) {
       return maybeThen<JSXOutput, JSXNodeInternal<string> | null>(jsx, (jsx) =>
-        findFirstStringJSX(jsx)
+        findFirstElementNode(jsx)
       );
     } else if (isSignal(jsx)) {
-      return findFirstStringJSX(untrack(() => jsx.value as JSXOutput));
+      return findFirstElementNode(untrack(() => jsx.value as JSXOutput));
     }
   }
   return null;
 }
 
-function addScriptNodeForInvisibleComponents(
+/**
+ * Injects a placeholder <script> element into the JSX output.
+ *
+ * This is necessary for headless components (components that don't render a real DOM element) to
+ * have an anchor point for `useOn` event listeners that target the document or window.
+ *
+ * @param jsx The JSX output to modify.
+ * @returns A tuple containing the created placeholder element and the modified JSX output.
+ */
+function injectPlaceholderElement(
   jsx: JSXOutput
 ): [JSXNodeInternal<string> | null, JSXOutput | null] {
+  // For regular JSX nodes, we can append the placeholder to its children.
   if (isJSXNode(jsx)) {
-    const jsxElement = createScriptNode();
+    const placeholder = createPlaceholderScriptNode();
+    // For slots, we can't add children, so we wrap them in a fragment.
     if (jsx.type === Slot) {
-      return [jsxElement, _jsxSorted(Fragment, null, null, [jsx, jsxElement], 0, null)];
+      return [placeholder, _jsxSorted(Fragment, null, null, [jsx, placeholder], 0, null)];
     }
 
     if (jsx.children == null) {
-      jsx.children = jsxElement;
+      jsx.children = placeholder;
     } else if (isArray(jsx.children)) {
-      jsx.children.push(jsxElement);
+      jsx.children.push(placeholder);
     } else {
-      jsx.children = [jsx.children, jsxElement];
+      jsx.children = [jsx.children, placeholder];
     }
-    return [jsxElement, jsx];
-  } else if (isArray(jsx) && jsx.length) {
-    // get first element
-    const [jsxElement, _] = addScriptNodeForInvisibleComponents(jsx[0]);
-    return [jsxElement, jsx];
-  } else if (isPrimitive(jsx)) {
-    const jsxElement = createScriptNode();
-    return [jsxElement, _jsxSorted(Fragment, null, null, [jsx, jsxElement], 0, null)];
+    return [placeholder, jsx];
   }
 
+  // For primitives, we can't add children, so we wrap them in a fragment.
+  if (isPrimitive(jsx)) {
+    const placeholder = createPlaceholderScriptNode();
+    return [placeholder, _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) {
+    const [createdElement, _] = injectPlaceholderElement(jsx[0]);
+    return [createdElement, jsx];
+  }
+
+  // For anything else we do nothing.
   return [null, jsx];
 }
 
-function createScriptNode(): JSXNodeInternal<string> {
+/**
+ * Creates a <script> element with a placeholder type.
+ *
+ * @returns A <script> element with a placeholder type.
+ */
+function createPlaceholderScriptNode(): JSXNodeInternal<string> {
   return new JSXNodeImpl(
     'script',
     {},

packages/qwik/src/core/shared/utils/types.ts

@@ -24,15 +24,10 @@ export const isFunction = <T extends (...args: any) => any>(v: unknown): v is T
   return typeof v === 'function';
 };
 
-export const isPrimitive = (v: unknown): v is string | number | boolean | null | undefined => {
-  return (
-    v === null ||
-    v === undefined ||
-    typeof v === 'string' ||
-    typeof v === 'number' ||
-    typeof v === 'boolean' ||
-    typeof v === 'symbol'
-  );
+export const isPrimitive = (
+  v: unknown
+): v is string | number | boolean | null | undefined | symbol => {
+  return typeof v !== 'object' && typeof v !== 'function' && v !== null && v !== undefined;
 };
 
 /**

packages/qwik/src/core/tests/use-on.spec.tsx

@@ -809,31 +809,47 @@ describe.each([
   });
 
   describe('regression', () => {
-    it('#7230 - when multiple useOn are used in a component that is not rendered, it should add multiple script nodes', async () => {
-      const BreakpointProvider = component$(() => {
+    it('#7230 - when multiple useOn are used in a component that is headless, it should still execute the events', async () => {
+      (globalThis as any).counter = 0;
+
+      const Cmp = component$(() => {
         useOnDocument(
           'click',
-          $(() => {})
+          $(() => {
+            (globalThis as any).counter++;
+          })
         );
 
         useOnWindow(
           'resize',
-          $(() => {})
+          $(() => {
+            (globalThis as any).counter++;
+          })
         );
 
-        useVisibleTask$(() => {});
+        useVisibleTask$(() => {
+          (globalThis as any).counter++;
+        });
 
         return <Slot />;
       });
 
       const LayoutTest = component$(() => {
         return (
-          <BreakpointProvider>
+          <Cmp>
             <div>test</div>
-          </BreakpointProvider>
+          </Cmp>
         );
       });
-      const { vNode } = await render(<LayoutTest />, { debug });
+      const { vNode, document } = await render(<LayoutTest />, { debug });
+      if (render === ssrRenderToDom) {
+        await trigger(document.body, 'script', ':document:qinit');
+      }
+      await trigger(document.body, 'script', ':document:click');
+      await trigger(document.body, 'script', ':window:resize');
+      expect((globalThis as any).counter).toBe(3);
+
+      (globalThis as any).counter = undefined;
       expect(vNode).toMatchVDOM(
         <Component ssr-required>
           <Component ssr-required>
@@ -842,8 +858,6 @@ describe.each([
                 <div>test</div>
               </Component>
               <script type="placeholder" hidden></script>
-              <script type="placeholder" hidden></script>
-              <script type="placeholder" hidden></script>
             </Component>
           </Component>
         </Component>