Merge pull request #7672 from QwikDev/v2-reuse-props-instance

fix: reuse the same props instance when props are changing

Author: wmertens

.changeset/cyan-walls-sing.md

@@ -0,0 +1,5 @@
+---
+'@qwik.dev/core': patch
+---
+
+fix: reuse the same props instance when props are changing

packages/qwik/src/core/client/vnode-diff.ts

@@ -9,6 +9,7 @@ import {
   directGetPropsProxyProp,
   isJSXNode,
   type Props,
+  type PropsProxy,
 } from '../shared/jsx/jsx-runtime';
 import { Slot } from '../shared/jsx/slot.public';
 import type { JSXNodeInternal, JSXOutput } from '../shared/jsx/types/jsx-node';
@@ -103,6 +104,7 @@ import { getFileLocationFromJsx } from '../shared/utils/jsx-filename';
 import { EffectProperty } 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';
 
 export const vnode_diff = (
   container: ClientContainer,
@@ -1063,16 +1065,36 @@ export const vnode_diff = (
       }
 
       if (host) {
-        const vNodeProps = vnode_getProp<any>(host, ELEMENT_PROPS, container.$getObjectById$);
-        shouldRender = shouldRender || propsDiffer(jsxProps, vNodeProps);
+        let vNodeProps = vnode_getProp<any>(host, ELEMENT_PROPS, container.$getObjectById$);
+        const propsAreDifferent = propsDiffer(jsxProps, vNodeProps);
+        shouldRender = shouldRender || propsAreDifferent;
         if (shouldRender) {
+          if (propsAreDifferent) {
+            if (vNodeProps) {
+              // Reuse the same props instance, qrls can use the current props instance
+              // as a capture ref, so we can't change it.
+              // We need to do this directly, because normally we would subscribe to the signals
+              // if any signal is there.
+              vNodeProps[_CONST_PROPS] = (jsxProps as PropsProxy)[_CONST_PROPS];
+              vNodeProps[_VAR_PROPS] = (jsxProps as PropsProxy)[_VAR_PROPS];
+            } else if (jsxProps) {
+              // If there is no props instance, create a new one.
+              // We can do this because we are not using the props instance for anything else.
+              vnode_setProp(host, ELEMENT_PROPS, jsxProps);
+              vNodeProps = jsxProps;
+            }
+          }
+          // Assign the new QRL instance to the host.
+          // Unfortunately it is created every time, something to fix in the optimizer.
+          vnode_setProp(host, OnRenderProp, componentQRL);
+
           /**
            * Mark host as not deleted. The host could have been marked as deleted if it there was a
            * cleanup run. Now we found it and want to reuse it, so we need to mark it as not
            * deleted.
            */
           host[VNodeProps.flags] &= ~VNodeFlags.Deleted;
-          container.$scheduler$(ChoreType.COMPONENT, host, componentQRL, jsxProps);
+          container.$scheduler$(ChoreType.COMPONENT, host, componentQRL, vNodeProps);
         }
       }
       descendContentToProject(jsxNode.children, host);

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

@@ -98,7 +98,6 @@ export const executeComponent = (
         if (!isInlineComponent) {
           container.setHostProp(renderHost, ELEMENT_SEQ_IDX, null);
           container.setHostProp(renderHost, USE_ON_LOCAL_SEQ_IDX, null);
-          container.setHostProp(renderHost, ELEMENT_PROPS, props);
         }
 
         if (vnode_isVNode(renderHost)) {

packages/qwik/src/core/shared/scheduler.ts

@@ -129,6 +129,7 @@ import { ComputedSignalImpl } from '../reactive-primitives/impl/computed-signal-
 import { WrappedSignalImpl } from '../reactive-primitives/impl/wrapped-signal-impl';
 import type { StoreHandler } from '../reactive-primitives/impl/store';
 import { SignalImpl } from '../reactive-primitives/impl/signal-impl';
+import { isQrl } from './qrl/qrl-utils';
 
 // Turn this on to get debug output of what the scheduler is doing.
 const DEBUG: boolean = false;
@@ -587,7 +588,10 @@ export const createScheduler = (
     }
 
     // If the host is the same (or missing), and the type is the same,  we need to compare the target.
-    if (a.$target$ !== b.$target$ || a.$payload$ !== b.$payload$) {
+    if (a.$target$ !== b.$target$) {
+      if (isQrl(a.$target$) && isQrl(b.$target$) && a.$target$.$hash$ === b.$target$.$hash$) {
+        return 0;
+      }
       // 1 means that we are going to process chores as FIFO
       return 1;
     }
@@ -643,7 +647,7 @@ export const createScheduler = (
      * multiple times during component execution. For this reason it is necessary for us to update
      * the chore with the latest result of the signal.
      */
-    if (existing.$type$ === ChoreType.NODE_DIFF) {
+    if (existing.$payload$ !== value.$payload$) {
       existing.$payload$ = value.$payload$;
     }
     if (existing.$executed$) {

packages/qwik/src/core/tests/component.spec.tsx

@@ -39,6 +39,8 @@ function Hola(props: any) {
   return <div {...props}></div>;
 }
 
+const globalObj = ['foo', 'bar'];
+
 describe.each([
   { render: ssrRenderToDom }, //
   { render: domRender }, //
@@ -1922,14 +1924,14 @@ describe.each([
     });
 
     const Cmp = component$(() => {
-      const $toggled = useSignal<boolean>(false);
+      const toggled = useSignal<boolean>(false);
 
       return (
         <TestB
-          aria-label={$toggled.value ? 'a' : 'a1'}
-          title={$toggled.value ? 'a' : 'a1'}
+          aria-label={toggled.value ? 'a' : 'a1'}
+          title={toggled.value ? 'a' : 'a1'}
           onClick$={() => {
-            $toggled.value = !$toggled.value;
+            toggled.value = !toggled.value;
           }}
         >
           <span>Hello, World!</span>
@@ -2255,6 +2257,80 @@ describe.each([
     expect(vnode_getProp(vnode_getParent(h1Element)!, OnRenderProp, null)).toBeNull();
   });
 
+  it('should reuse the same props instance when props are changing', async () => {
+    (globalThis as any).logs = [];
+    type ChildProps = {
+      obj: string;
+      foo: SignalType<number>;
+    };
+    const Child = component$<ChildProps>(({ obj, foo }) => {
+      (globalThis as any).logs.push('child render ' + obj);
+      useTask$(({ track }) => {
+        foo && track(foo);
+        (globalThis as any).logs.push(obj);
+      });
+      return <></>;
+    });
+
+    const Cmp = component$(() => {
+      const foo = useSignal(0);
+      (globalThis as any).logs.push('parent render');
+      return (
+        <div>
+          <button
+            onClick$={() => {
+              foo.value === 0 ? (foo.value = 1) : (foo.value = 0);
+            }}
+          >
+            click
+          </button>
+          <Child obj={globalObj[foo.value]} foo={foo} />
+        </div>
+      );
+    });
+
+    const { document } = await render(<Cmp />, { debug });
+
+    await trigger(document.body, 'button', 'click');
+
+    expect((globalThis as any).logs).toEqual([
+      'parent render',
+      'child render foo',
+      'foo',
+      'parent render',
+      'bar',
+      'child render bar',
+    ]);
+
+    (globalThis as any).logs = undefined;
+  });
+
+  it('should change component props to new one for the same component with the same key', async () => {
+    (globalThis as any).logs = [];
+    const FirstCmp = component$((props: { foo?: string; bar?: string }) => {
+      (globalThis as any).logs.push('foo' in props, 'bar' in props);
+      return <div>{props.foo}</div>;
+    });
+
+    const Cmp = component$(() => {
+      const toggle = useSignal(true);
+      return (
+        <>
+          <button onClick$={() => (toggle.value = !toggle.value)}></button>
+          {toggle.value ? <FirstCmp key="1" foo="foo" /> : <FirstCmp key="1" bar="bar" />}
+        </>
+      );
+    });
+
+    const { document } = await render(<Cmp />, { debug });
+    expect((globalThis as any).logs).toEqual([true, false]);
+    await trigger(document.body, 'button', 'click');
+    expect((globalThis as any).logs).toEqual([true, false, false, true]);
+    await trigger(document.body, 'button', 'click');
+    expect((globalThis as any).logs).toEqual([true, false, false, true, true, false]);
+    (globalThis as any).logs = undefined;
+  });
+
   describe('regression', () => {
     it('#3643', async () => {
       const Issue3643 = component$(() => {

starters/apps/e2e/src/components/render/render.tsx

@@ -13,7 +13,7 @@ import {
   type PropsOf,
   type QRL,
 } from "@qwik.dev/core";
-import { h, SSRComment, SSRRaw } from "@qwik.dev/core/internal";
+import { h, SSRComment, SSRRaw, type Signal } from "@qwik.dev/core/internal";
 import { delay } from "../streaming/demo";
 
 export const Render = component$(() => {
@@ -103,6 +103,7 @@ export const RenderChildren = component$<{ v: number }>(({ v }) => {
       <Issue4455 />
       <Issue5266 />
       <DynamicButton id="dynamic-button" />;
+      <RerenderOnce />
     </>
   );
 });
@@ -960,3 +961,32 @@ export const DynamicButton = component$<any>(
     );
   },
 );
+
+const globalObj = ["foo", "bar"];
+
+let logs: any[] = [];
+
+const RerenderOnceChild = component$<{ obj: string; foo: Signal<number> }>(
+  ({ obj, foo }) => {
+    logs.push("render Cmp", obj, foo.value);
+    return <span id="rerender-once-child">{JSON.stringify(logs)}</span>;
+  },
+);
+
+export const RerenderOnce = component$(() => {
+  const foo = useSignal(0);
+  logs = [];
+  return (
+    <div>
+      <button
+        id="rerender-once-button"
+        onClick$={() => {
+          foo.value === 0 ? (foo.value = 1) : (foo.value = 0);
+        }}
+      >
+        click
+      </button>
+      <RerenderOnceChild obj={globalObj[foo.value]} foo={foo} />
+    </div>
+  );
+});

starters/e2e/e2e.render.e2e.ts

@@ -12,7 +12,7 @@ test.describe("render", () => {
     });
   });
 
-  function tests() {
+  function tests(isClient: boolean) {
     test("should load", async ({ page }) => {
       const button = page.locator("button#increment");
       const text = page.locator("#rerenders");
@@ -486,9 +486,36 @@ test.describe("render", () => {
       await button.click();
       await expect(tag).toHaveAttribute("data-v", "bar");
     });
+
+    test("should rerender child once", async ({ page }) => {
+      const button = page.locator("#rerender-once-button");
+      expect(await page.locator("#rerender-once-child").innerHTML()).toEqual(
+        '["render Cmp","foo",0]',
+      );
+      await button.click();
+      if (isClient) {
+        await expect(page.locator("#rerender-once-child")).toHaveText(
+          '["render Cmp","foo",0,"render Cmp","bar",1]',
+        );
+      } else {
+        await expect(page.locator("#rerender-once-child")).toHaveText(
+          '["render Cmp","bar",1]',
+        );
+      }
+      await button.click();
+      if (isClient) {
+        await expect(page.locator("#rerender-once-child")).toHaveText(
+          '["render Cmp","foo",0,"render Cmp","bar",1,"render Cmp","foo",0]',
+        );
+      } else {
+        await expect(page.locator("#rerender-once-child")).toHaveText(
+          '["render Cmp","bar",1,"render Cmp","foo",0]',
+        );
+      }
+    });
   }
 
-  tests();
+  tests(false);
 
   test.describe("client rerender", () => {
     test.beforeEach(async ({ page }) => {
@@ -499,7 +526,7 @@ test.describe("render", () => {
         `Render ${Number(v) + 1}`,
       );
     });
-    tests();
+    tests(true);
   });
 
   test("pr3475", async ({ page }) => {

starters/e2e/e2e.signals.e2e.ts

@@ -450,8 +450,7 @@ test.describe("signals", () => {
       await expect(result).toHaveAttribute("data-value", "collision");
     });
 
-    // This test is flaky, we have it working well in v2
-    test.skip("issue 4228", async ({ page }) => {
+    test("issue 4228", async ({ page }) => {
       const buttonA = page.locator("#issue-4228-button-a");
       const buttonB = page.locator("#issue-4228-button-b");
       const buttonC = page.locator("#issue-4228-button-c");