feat(cursors): handle changing component props

Author: Varixo

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

@@ -1378,7 +1378,6 @@ function handleProps(
   container: ClientContainer
 ): boolean {
   let shouldRender = false;
-  let propsAreDifferent = false;
   if (vNodeProps) {
     const effects = vNodeProps[_PROPS_HANDLER].$effects$;
     const constPropsDifferent = handleChangedProps(
@@ -1388,33 +1387,24 @@ function handleProps(
       container,
       false
     );
-    propsAreDifferent = constPropsDifferent;
     shouldRender ||= constPropsDifferent;
     if (effects && effects.size > 0) {
-      const varPropsDifferent = handleChangedProps(
+      handleChangedProps(
         jsxProps[_VAR_PROPS],
         vNodeProps[_VAR_PROPS],
         vNodeProps[_PROPS_HANDLER],
-        container
+        container,
+        true
       );
-
-      propsAreDifferent ||= varPropsDifferent;
       // don't mark as should render, effects will take care of it
-      // shouldRender ||= varPropsDifferent;
-    }
-  }
-
-  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.
-      vNodeProps[_OWNER] = (jsxProps as PropsProxy)[_OWNER];
-    } 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 as VirtualVNode, ELEMENT_PROPS, jsxProps);
-      vNodeProps = jsxProps;
     }
+    // Update the owner after all props have been synced
+    vNodeProps[_OWNER] = (jsxProps as PropsProxy)[_OWNER];
+  } 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 as VirtualVNode, ELEMENT_PROPS, jsxProps);
+    vNodeProps = jsxProps;
   }
   return shouldRender;
 }
@@ -1426,51 +1416,49 @@ function handleChangedProps(
   container: ClientContainer,
   triggerEffects: boolean = true
 ): boolean {
-  const srcEmpty = isPropsEmpty(src);
-  const dstEmpty = isPropsEmpty(dst);
-
-  if (srcEmpty && dstEmpty) {
+  if (isPropsEmpty(src) && isPropsEmpty(dst)) {
     return false;
   }
 
-  if (srcEmpty || dstEmpty) {
-    return true;
-  }
-
-  const srcKeys = Object.keys(src!);
-  const dstKeys = Object.keys(dst!);
-
-  let srcLen = srcKeys.length;
-  let dstLen = dstKeys.length;
-  if ('children' in src!) {
-    srcLen--;
-  }
-  if (QBackRefs in src!) {
-    srcLen--;
-  }
-  if ('children' in dst!) {
-    dstLen--;
-  }
-  if (QBackRefs in dst!) {
-    dstLen--;
-  }
+  propsHandler.$container$ = container;
+  let changed = false;
 
-  if (srcLen !== dstLen) {
-    return true;
+  // Update changed/added props from src
+  if (src) {
+    for (const key in src) {
+      if (key === 'children' || key === QBackRefs) {
+        continue;
+      }
+      if (!dst || src[key] !== dst[key]) {
+        changed = true;
+        if (triggerEffects) {
+          if (dst) {
+            // Update the value in dst BEFORE triggering effects
+            // so effects see the new value
+            // Note: Value is not triggering effects, because we are modyfing direct VAR_PROPS object
+            dst[key] = src[key];
+          }
+          triggerPropsProxyEffect(propsHandler, key);
+        } else {
+          // Early return for const props (no effects)
+          return true;
+        }
+      }
+    }
   }
 
-  let changed = false;
-  propsHandler.$container$ = container;
-  for (const key of srcKeys) {
-    if (key === 'children' || key === QBackRefs) {
-      continue;
-    }
-    if (!Object.prototype.hasOwnProperty.call(dst, key) || src![key] !== dst![key]) {
-      changed = true;
-      if (triggerEffects) {
-        triggerPropsProxyEffect(propsHandler, key);
-      } else {
-        return true;
+  // Remove props that are in dst but not in src
+  if (dst) {
+    for (const key in dst) {
+      if (key === 'children' || key === QBackRefs) {
+        continue;
+      }
+      if (!src || !(key in src)) {
+        changed = true;
+        if (triggerEffects) {
+          delete dst[key];
+          triggerPropsProxyEffect(propsHandler, key);
+        }
       }
     }
   }

packages/qwik/src/core/shared/cursor/cursor-walker.ts

@@ -240,5 +240,6 @@ function getNextVNode(vNode: VNode): VNode | null {
   }
   // all array items checked, children are no longer dirty
   parent!.dirty &= ~ChoreBits.CHILDREN;
+  parent!.dirtyChildren = null;
   return getNextVNode(parent!);
 }

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

@@ -30,7 +30,7 @@ import { ErrorProvider } from '../../testing/rendering.unit-util';
 import * as qError from '../shared/error/error';
 import { QContainerValue } from '../shared/types';
 import { ELEMENT_PROPS, OnRenderProp, QContainerAttr } from '../shared/utils/markers';
-import { vnode_locate } from '../client/vnode';
+import { vnode_getProp, vnode_locate } from '../client/vnode';
 import type { PropsProxy } from '../shared/jsx/props-proxy';
 import { _PROPS_HANDLER } from '../shared/utils/constants';
 
@@ -2332,7 +2332,7 @@ describe.each([
     );
     const h1Element = vnode_locate(container.rootVNode, document.querySelector('h1')!);
 
-    expect(h1Element.parent!.getProp(OnRenderProp, null)).toBeNull();
+    expect(vnode_getProp(h1Element.parent!, OnRenderProp, null)).toBeNull();
   });
 
   it('should reuse the same props instance when props are changing', async () => {
@@ -2741,7 +2741,11 @@ describe.each([
       </Component>
     );
 
-    const propsProxy = vNode!.getProp<PropsProxy>(ELEMENT_PROPS, container.$getObjectById$)!;
+    const propsProxy = vnode_getProp<PropsProxy>(
+      vNode!,
+      ELEMENT_PROPS,
+      container.$getObjectById$ as ((id: string) => PropsProxy) | null
+    )!;
     expect(propsProxy[_PROPS_HANDLER]?.$effects$).toBeDefined();
     expect(propsProxy[_PROPS_HANDLER]?.$effects$?.size).toBe(1);