fix(v2): component rerender with inline component inside

QwikDev · Sep 23, 2024 · 9f8b88f · 9f8b88f
1 parent cf77a1f
commit 9f8b88f
Show file tree

Hide file tree

Showing 7 changed files with 136 additions and 26 deletions.
diff --git a/packages/qwik/src/core/render/jsx/jsx-runtime.ts b/packages/qwik/src/core/render/jsx/jsx-runtime.ts
@@ -271,6 +271,26 @@ export const isJSXNode = <T>(n: unknown): n is JSXNode<T> => {
   }
 };
 
+/**
+ * Instead of using PropsProxyHandler getter (which could create a component-level subscription).
+ * Use this function to get the props directly from a const or var props.
+ */
+export const directGetPropsProxyProp = <T>(
+  varProps: Props,
+  constProps: Props | null,
+  prop: string
+): T => {
+  return (constProps && prop in constProps ? constProps[prop] : varProps[prop]) as T;
+};
+
+export const getMergedProps = (props: Props): Props => {
+  return {
+    ...(props as any)[_VAR_PROPS],
+    ...(props as any)[_CONST_PROPS],
+    children: props.children,
+  };
+};
+
 /** @public */
 export const Fragment: FunctionComponent<{ children?: any; key?: string | number | null }> = (
   props
@@ -357,10 +377,7 @@ class PropsProxyHandler implements ProxyHandler<any> {
     if (this.$children$ != null && prop === 'children') {
       return this.$children$;
     }
-    const value =
-      this.$constProps$ && prop in this.$constProps$
-        ? this.$constProps$[prop as string]
-        : this.$varProps$[prop as string];
+    const value = directGetPropsProxyProp(this.$varProps$, this.$constProps$, prop as string);
     // a proxied value that the optimizer made
     return value instanceof WrappedSignal ? value.value : value;
   }

diff --git a/packages/qwik/src/core/v2/client/vnode-diff.ts b/packages/qwik/src/core/v2/client/vnode-diff.ts
@@ -5,7 +5,14 @@ import { assertDefined, assertFalse, assertTrue } from '../../error/assert';
 import type { QRLInternal } from '../../qrl/qrl-class';
 import type { QRL } from '../../qrl/qrl.public';
 import { dangerouslySetInnerHTML, serializeAttribute } from '../../render/execute-component';
-import { Fragment, JSXNodeImpl, isJSXNode, type Props } from '../../render/jsx/jsx-runtime';
+import {
+  Fragment,
+  JSXNodeImpl,
+  directGetPropsProxyProp,
+  getMergedProps,
+  isJSXNode,
+  type Props,
+} from '../../render/jsx/jsx-runtime';
 import { Slot } from '../../render/jsx/slot.public';
 import type { JSXNode, JSXOutput } from '../../render/jsx/types/jsx-node';
 import type { JSXChildren } from '../../render/jsx/types/jsx-qwik-attributes';
@@ -425,7 +432,10 @@ export const vnode_diff = (
       /// STEP 1: Bucketize the children based on the projection name.
       for (let i = 0; i < children.length; i++) {
         const child = children[i];
-        const slotName = String((isJSXNode(child) && child.props[QSlot]) || QDefaultSlot);
+        const slotName = String(
+          (isJSXNode(child) && directGetPropsProxyProp(child.varProps, child.constProps, QSlot)) ||
+            QDefaultSlot
+        );
         const idx = mapApp_findIndx(projections, slotName, 0);
         let jsxBucket: JSXNodeImpl<typeof Projection>;
         if (idx >= 0) {
@@ -529,7 +539,7 @@ export const vnode_diff = (
         );
       }
     }
-    return jsxValue.props.name || QDefaultSlot;
+    return directGetPropsProxyProp(jsxValue.varProps, jsxValue.constProps, 'name') || QDefaultSlot;
   }
 
   function drainAsyncQueue(): ValueOrPromise<void> {
@@ -987,16 +997,17 @@ export const vnode_diff = (
   function expectComponent(component: Function) {
     const componentMeta = (component as any)[SERIALIZABLE_STATE] as [QRLInternal<OnRenderFn<any>>];
     let host = (vNewNode || vCurrent) as VirtualVNode | null;
+    const currentJSX: JSXNode = jsxValue;
     if (componentMeta) {
-      const jsxProps = jsxValue.props;
+      const jsxProps = currentJSX.props;
       // QComponent
       let shouldRender = false;
       const [componentQRL] = componentMeta;
 
       const componentHash = componentQRL.$hash$;
       const vNodeComponentHash = getComponentHash(host, container.$getObjectById$);
 
-      const lookupKey = jsxValue.key || componentHash;
+      const lookupKey = currentJSX.key || componentHash;
       const vNodeLookupKey = getKey(host) || vNodeComponentHash;
 
       const lookupKeysAreEqual = lookupKey === vNodeLookupKey;
@@ -1033,7 +1044,7 @@ export const vnode_diff = (
           container.$scheduler$(ChoreType.COMPONENT, host, componentQRL, jsxProps);
         }
       }
-      jsxValue.children != null && descendContentToProject(jsxValue.children, host);
+      currentJSX.children != null && descendContentToProject(currentJSX.children, host);
     } else {
       // Inline Component
       vnode_insertBefore(
@@ -1043,7 +1054,7 @@ export const vnode_diff = (
         vCurrent && getInsertBefore()
       );
       isDev && vnode_setProp(vNewNode, DEBUG_TYPE, VirtualType.InlineComponent);
-      vnode_setProp(vNewNode, ELEMENT_PROPS, jsxValue.props);
+      vnode_setProp(vNewNode, ELEMENT_PROPS, currentJSX.props);
 
       host = vNewNode;
       let component$Host: VNode | null = host;
@@ -1061,7 +1072,7 @@ export const vnode_diff = (
         host,
         (component$Host || container.rootVNode) as HostElement,
         component as OnRenderFn<unknown>,
-        jsxValue.props
+        getMergedProps(currentJSX.props)
       );
       asyncQueue.push(jsxOutput, host);
     }

diff --git a/packages/qwik/src/core/v2/shared/component-execution.ts b/packages/qwik/src/core/v2/shared/component-execution.ts
@@ -2,7 +2,7 @@ import { isDev } from '@builder.io/qwik/build';
 import { isQwikComponent, type OnRenderFn } from '../../component/component.public';
 import { assertDefined } from '../../error/assert';
 import { isQrl, type QRLInternal } from '../../qrl/qrl-class';
-import { JSXNodeImpl, isJSXNode } from '../../render/jsx/jsx-runtime';
+import { JSXNodeImpl, isJSXNode, type Props } from '../../render/jsx/jsx-runtime';
 import type { JSXNode, JSXOutput } from '../../render/jsx/types/jsx-node';
 import type { KnownEventNames } from '../../render/jsx/types/jsx-qwik-events';
 import { invokeApply, newInvokeContext, untrack } from '../../use/use-core';
@@ -23,6 +23,7 @@ import { logWarn } from '../../util/log';
 import { EffectProperty, isSignal } from '../signal/v2-signal';
 import { vnode_isVNode } from '../client/vnode';
 import { clearVNodeEffectDependencies } from '../signal/v2-subscriber';
+import { _CONST_PROPS, _VAR_PROPS } from '../../internal';
 
 /**
  * Use `executeComponent2` to execute a component.
@@ -50,7 +51,7 @@ export const executeComponent2 = (
   renderHost: HostElement,
   subscriptionHost: HostElement,
   componentQRL: OnRenderFn<unknown> | QRLInternal<OnRenderFn<unknown>> | null,
-  props: Record<string, unknown> | null
+  props: Props | null
 ): ValueOrPromise<JSXOutput> => {
   const iCtx = newInvokeContext(
     container.$locale$,
@@ -74,7 +75,7 @@ export const executeComponent2 = (
     componentFn = componentQRL.getFn(iCtx);
   } else if (isQwikComponent(componentQRL)) {
     const qComponentFn = componentQRL as (
-      props: Record<string, unknown>,
+      props: Props,
       key: string | null,
       flags: number
     ) => JSXNode;

diff --git a/packages/qwik/src/core/v2/ssr/ssr-render-component.ts b/packages/qwik/src/core/v2/ssr/ssr-render-component.ts
@@ -8,6 +8,7 @@ import { executeComponent2 } from '../shared/component-execution';
 import { ChoreType } from '../shared/scheduler';
 import type { ValueOrPromise } from '../../util/types';
 import type { JSXOutput } from '../../render/jsx/types/jsx-node';
+import { getMergedProps } from '../../render/jsx/jsx-runtime';
 
 export const applyInlineComponent = (
   ssr: SSRContainer,
@@ -16,7 +17,7 @@ export const applyInlineComponent = (
   jsx: JSXNode
 ) => {
   const host = ssr.getLastNode();
-  return executeComponent2(ssr, host, component$Host, component, jsx.props);
+  return executeComponent2(ssr, host, component$Host, component, getMergedProps(jsx.props));
 };
 
 export const applyQwikComponentBody = (

diff --git a/packages/qwik/src/core/v2/ssr/ssr-render-jsx.ts b/packages/qwik/src/core/v2/ssr/ssr-render-jsx.ts
@@ -3,7 +3,7 @@ import { isQwikComponent } from '../../component/component.public';
 import { isQrl } from '../../qrl/qrl-class';
 import type { QRL } from '../../qrl/qrl.public';
 import { serializeAttribute } from '../../render/execute-component';
-import { Fragment } from '../../render/jsx/jsx-runtime';
+import { Fragment, directGetPropsProxyProp } from '../../render/jsx/jsx-runtime';
 import { Slot } from '../../render/jsx/slot.public';
 import type { JSXNode, JSXOutput } from '../../render/jsx/types/jsx-node';
 import type { JSXChildren } from '../../render/jsx/types/jsx-qwik-attributes';
@@ -26,7 +26,7 @@ import {
   isJsxPropertyAnEventName,
   isPreventDefault,
 } from '../shared/event-names';
-import { addComponentStylePrefix, hasClassAttr, isClassAttr } from '../shared/scoped-styles';
+import { addComponentStylePrefix, isClassAttr } from '../shared/scoped-styles';
 import { qrlToString, type SerializationContext } from '../shared/shared-serialization';
 import { DEBUG_TYPE, VirtualType, type fixMeAny } from '../shared/types';
 import { WrappedSignal, EffectProperty, isSignal } from '../signal/v2-signal';
@@ -157,8 +157,7 @@ function processJSXNode(
       // Below, JSXChildren allows functions and regexes, but we assume the dev only uses those as appropriate.
       if (typeof type === 'string') {
         // append class attribute if styleScopedId exists and there is no class attribute
-        const classAttributeExists =
-          hasClassAttr(jsx.varProps) || (jsx.constProps && hasClassAttr(jsx.constProps));
+        const classAttributeExists = directGetPropsProxyProp(jsx.varProps, jsx.constProps, 'class');
         if (!classAttributeExists && styleScoped) {
           if (!jsx.constProps) {
             jsx.constProps = {};
@@ -233,7 +232,7 @@ function processJSXNode(
             ssr.closeFragment();
           }
         } else if (type === SSRComment) {
-          ssr.commentNode((jsx.props.data as string) || '');
+          ssr.commentNode(directGetPropsProxyProp(jsx.varProps, jsx.constProps, 'data') || '');
         } else if (type === SSRStream) {
           ssr.commentNode(FLUSH_COMMENT);
           const generator = jsx.children as SSRStreamChildren;
@@ -252,7 +251,7 @@ function processJSXNode(
           enqueue(value as StackValue);
           isPromise(value) && enqueue(Promise);
         } else if (type === SSRRaw) {
-          ssr.htmlNode(jsx.props.data as string);
+          ssr.htmlNode(directGetPropsProxyProp(jsx.varProps, jsx.constProps, 'data'));
         } else if (isQwikComponent(type)) {
           // prod: use new instance of an array for props, we always modify props for a component
           ssr.openComponent(isDev ? [DEBUG_TYPE, VirtualType.Component] : []);
@@ -491,14 +490,17 @@ function getSlotName(host: ISsrNode, jsx: JSXNode, ssr: SSRContainer): string {
       return trackSignal(() => constValue.value, host as fixMeAny, EffectProperty.COMPONENT, ssr);
     }
   }
-  return (jsx.props.name as string) || QDefaultSlot;
+  return directGetPropsProxyProp(jsx.varProps, jsx.constProps, 'name') || QDefaultSlot;
 }
 
 function appendQwikInspectorAttribute(jsx: JSXNode) {
   if (isDev && qInspector && jsx.dev && jsx.type !== 'head') {
     const sanitizedFileName = jsx.dev.fileName?.replace(/\\/g, '/');
     const qwikInspectorAttr = 'data-qwik-inspector';
-    if (sanitizedFileName && !(qwikInspectorAttr in jsx.props)) {
+    if (
+      sanitizedFileName &&
+      !directGetPropsProxyProp(jsx.varProps, jsx.constProps, qwikInspectorAttr)
+    ) {
       if (!jsx.constProps) {
         jsx.constProps = {};
       }

diff --git a/packages/qwik/src/core/v2/tests/inline-component.spec.tsx b/packages/qwik/src/core/v2/tests/inline-component.spec.tsx
@@ -4,6 +4,7 @@ import {
   Fragment as InlineComponent,
   component$,
   useSignal,
+  useStore,
 } from '@builder.io/qwik';
 import { domRender, ssrRenderToDom, trigger } from '@builder.io/qwik/testing';
 import { describe, expect, it } from 'vitest';
@@ -27,6 +28,8 @@ const InlineWrapper = () => {
   return <MyComp />;
 };
 
+const Id = (props: any) => <div>Id: {props.id}</div>;
+
 describe.each([
   { render: ssrRenderToDom }, //
   { render: domRender }, //
@@ -147,4 +150,80 @@ describe.each([
       </Component>
     );
   });
+
+  it('should not rerender component', async () => {
+    const Child = component$((props: { id: number }) => {
+      const renders = useStore(
+        {
+          count: 0,
+        },
+        { reactive: false }
+      );
+      renders.count++;
+      const rerenders = renders.count + 0;
+      return (
+        <>
+          <Id id={props.id} />
+          {rerenders}
+        </>
+      );
+    });
+    const Cmp = component$(() => {
+      const id = useSignal(0);
+      return (
+        <>
+          <button
+            onClick$={() => {
+              id.value++;
+            }}
+          >
+            Increment ID
+          </button>
+          <Child id={id.value} />
+        </>
+      );
+    });
+
+    const { vNode, container } = await render(<Cmp />, { debug });
+
+    expect(vNode).toMatchVDOM(
+      <Component>
+        <>
+          <button>Increment ID</button>
+          <Component>
+            <Fragment>
+              <InlineComponent>
+                <div>
+                  {'Id: '}
+                  <Fragment>{'0'}</Fragment>
+                </div>
+              </InlineComponent>
+              1
+            </Fragment>
+          </Component>
+        </>
+      </Component>
+    );
+
+    await trigger(container.document.body, 'button', 'click');
+
+    expect(vNode).toMatchVDOM(
+      <Component>
+        <>
+          <button>Increment ID</button>
+          <Component>
+            <Fragment>
+              <InlineComponent>
+                <div>
+                  {'Id: '}
+                  <Fragment>{'1'}</Fragment>
+                </div>
+              </InlineComponent>
+              1
+            </Fragment>
+          </Component>
+        </>
+      </Component>
+    );
+  });
 });
diff --git a/starters/e2e/e2e.signals.e2e.ts b/starters/e2e/e2e.signals.e2e.ts
@@ -12,8 +12,7 @@ test.describe("signals", () => {
   });
 
   function tests() {
-    // TODO(v2): fix this
-    test.skip("should do its thing", async ({ page }) => {
+    test("should do its thing", async ({ page }) => {
       const incrementBtn = page.locator("#count");
       const clickBtn = page.locator("#click");
       const incrementIdBtn = page.locator("#increment-id");