fix: set const and var props directly, compare qrl hashes

Author: Varixo

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,29 +1065,36 @@ export const vnode_diff = (
       }
 
       if (host) {
-        const vNodeProps = vnode_getProp<any>(host, ELEMENT_PROPS, container.$getObjectById$);
+        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.
-              Object.assign(vNodeProps, jsxProps);
+              // 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.
+              // 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);
+              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/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;
@@ -588,6 +589,9 @@ 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$) {
+      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;
     }

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

@@ -2305,6 +2305,32 @@ describe.each([
     (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 }) => {