fix: reuse the same props instance when props are changing

Author: Varixo

.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

@@ -1064,8 +1064,21 @@ export const vnode_diff = (
 
       if (host) {
         const vNodeProps = vnode_getProp<any>(host, ELEMENT_PROPS, container.$getObjectById$);
-        shouldRender = shouldRender || propsDiffer(jsxProps, vNodeProps);
+        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.
+              Object.assign(vNodeProps, jsxProps);
+            } 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.
+              container.setHostProp(host, ELEMENT_PROPS, jsxProps);
+            }
+          }
+
           /**
            * 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

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

@@ -587,7 +587,7 @@ 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$) {
       // 1 means that we are going to process chores as FIFO
       return 1;
     }
@@ -643,7 +643,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,54 @@ 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;
+  });
+
   describe('regression', () => {
     it('#3643', async () => {
       const Issue3643 = component$(() => {

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");