Merge pull request #7259 from QwikDev/v2-retry-vnode-diff-on-promise-throw

fix: retry vnode diffing on promise throw

Author: Varixo

.changeset/slimy-weeks-hope.md

@@ -0,0 +1,5 @@
+---
+'@qwik.dev/core': patch
+---
+
+fix: retry vnode diffing on promise throw

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

@@ -345,11 +345,13 @@ export const createScheduler = (
           (jsx) => {
             if (chore.$type$ === ChoreType.COMPONENT) {
               const styleScopedId = container.getHostProp<string>(host, QScopedStyle);
-              return vnode_diff(
-                container as ClientContainer,
-                jsx,
-                host as VirtualVNode,
-                addComponentStylePrefix(styleScopedId)
+              return retryOnPromise(() =>
+                vnode_diff(
+                  container as ClientContainer,
+                  jsx,
+                  host as VirtualVNode,
+                  addComponentStylePrefix(styleScopedId)
+                )
               );
             } else {
               return jsx;
@@ -382,7 +384,9 @@ export const createScheduler = (
         if (isSignal(jsx)) {
           jsx = jsx.value as any;
         }
-        returnValue = vnode_diff(container as DomContainer, jsx, parentVirtualNode, null);
+        returnValue = retryOnPromise(() =>
+          vnode_diff(container as DomContainer, jsx, parentVirtualNode, null)
+        );
         break;
       case ChoreType.NODE_PROP:
         const virtualNode = chore.$host$ as unknown as ElementVNode;

packages/qwik/src/core/shared/utils/promises.ts

@@ -106,13 +106,26 @@ export const delay = (timeout: number) => {
   });
 };
 
-export function retryOnPromise<T>(fn: () => T, retryCount: number = 0): ValueOrPromise<T> {
-  try {
-    return fn();
-  } catch (e) {
+// Retries a function that throws a promise.
+export function retryOnPromise<T>(
+  fn: () => ValueOrPromise<T>,
+  retryCount: number = 0
+): ValueOrPromise<T> {
+  const retryOrThrow = (e: any) => {
     if (isPromise(e) && retryCount < MAX_RETRY_ON_PROMISE_COUNT) {
       return e.then(retryOnPromise.bind(null, fn, retryCount++)) as ValueOrPromise<T>;
     }
     throw e;
+  };
+
+  try {
+    const result = fn();
+    if (isPromise(result)) {
+      // not awaited promise is not caught by try/catch block
+      return result.catch((e) => retryOrThrow(e));
+    }
+    return result;
+  } catch (e) {
+    return retryOrThrow(e);
   }
 }

packages/qwik/src/core/signal/signal-subscriber.ts

@@ -9,6 +9,8 @@ import {
 import { EffectSubscriptionsProp, WrappedSignal, isSignal, type Signal } from './signal';
 import type { Container } from '../shared/types';
 import { StoreHandler, getStoreHandler, isStore, type TargetType } from './store';
+import { isPropsProxy } from '../shared/jsx/jsx-runtime';
+import { _CONST_PROPS, _VAR_PROPS } from '../internal';
 
 export abstract class Subscriber {
   $effectDependencies$: (Subscriber | TargetType)[] | null = null;
@@ -144,6 +146,20 @@ function clearArgEffect(arg: any, subscriber: Subscriber, seenSet: Set<unknown>)
   } else if (typeof arg === 'object' && arg !== null) {
     if (isStore(arg)) {
       clearStoreEffects(getStoreHandler(arg)!, subscriber);
+    } else if (isPropsProxy(arg)) {
+      // Separate check for props proxy, because props proxy getter could call signal getter.
+      // To avoid that we need to get the constProps and varProps directly
+      // from the props proxy object and loop over them.
+      const constProps = arg[_CONST_PROPS];
+      const varProps = arg[_VAR_PROPS];
+      if (constProps) {
+        for (const key in constProps) {
+          clearArgEffect(constProps[key], subscriber, seenSet);
+        }
+      }
+      for (const key in varProps) {
+        clearArgEffect(varProps[key], subscriber, seenSet);
+      }
     } else {
       for (const key in arg) {
         clearArgEffect(arg[key], subscriber, seenSet);

packages/qwik/src/core/tests/render-promise.spec.tsx

@@ -1,4 +1,4 @@
-import { Fragment as Component, Fragment, component$ } from '@qwik.dev/core';
+import { Fragment as Component, Fragment, component$, useSignal } from '@qwik.dev/core';
 import { domRender, ssrRenderToDom } from '@qwik.dev/core/testing';
 import { describe, expect, it } from 'vitest';
 
@@ -24,4 +24,23 @@ describe.each([
       </Component>
     );
   });
+
+  it('should handle thrown Promise', async () => {
+    const Child = component$(() => {
+      const signal = useSignal(0);
+      if (signal.value === 0) {
+        throw new Promise((r) => r(signal.value++));
+      }
+      return 'child';
+    });
+    const Cmp = component$(() => {
+      return (
+        <div>
+          <Child />
+        </div>
+      );
+    });
+    const { document } = await render(<Cmp />, { debug });
+    expect(document.querySelector('div')).toHaveProperty('textContent', 'child');
+  });
 });

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

@@ -1,4 +1,10 @@
-import { component$, useSignal, useStore } from "@qwik.dev/core";
+import {
+  component$,
+  useComputed$,
+  useSignal,
+  useStore,
+  type PropsOf,
+} from "@qwik.dev/core";
 
 export const Attributes = component$(() => {
   const render = useSignal(0);
@@ -13,6 +19,7 @@ export const Attributes = component$(() => {
         Rerender
       </button>
       <AttributesChild v={render.value} key={render.value} />
+      <ProgressParent />
     </>
   );
 });
@@ -214,3 +221,90 @@ export const Issue4718Null = component$(() => {
     </button>
   );
 });
+
+const ProgressRoot = component$<{ min?: number } & PropsOf<"div">>((props) => {
+  const { ...rest } = props;
+
+  const minSig = useComputed$(() => props.min ?? 0);
+
+  const valueLabelSig = useComputed$(() => {
+    const value = minSig.value;
+    return `${value * 100}%`;
+  });
+
+  return (
+    <>
+      <div id="progress-1" aria-valuetext={valueLabelSig.value} {...rest}>
+        {valueLabelSig.value}
+      </div>
+    </>
+  );
+});
+
+const ProgressRootShowHide = component$<{ min: number } & PropsOf<"div">>(
+  ({ min, ...rest }) => {
+    const show = useSignal(true);
+
+    return (
+      <>
+        {show.value && (
+          <div id="progress-2" aria-valuetext={min.toString()} {...rest}>
+            {min}
+          </div>
+        )}
+
+        <button id="progress-hide" onClick$={() => (show.value = !show.value)}>
+          show/hide progress
+        </button>
+      </>
+    );
+  },
+);
+
+const ProgressRootPromise = component$<{ min?: number } & PropsOf<"div">>(
+  (props) => {
+    const { ...rest } = props;
+
+    const minSig = useComputed$(() => props.min ?? 0);
+
+    const valueLabelSig = useComputed$(() => {
+      const value = minSig.value;
+      return `${value * 100}%`;
+    });
+
+    return (
+      <>
+        {Promise.resolve(
+          <div id="progress-3" aria-valuetext={valueLabelSig.value} {...rest}>
+            {valueLabelSig.value}
+          </div>,
+        )}
+      </>
+    );
+  },
+);
+
+const ProgressParent = component$(() => {
+  const minGoal = useSignal(2000);
+  const computedGoal = useComputed$(() => minGoal.value + 100);
+
+  return (
+    <div>
+      <div>
+        <span id="progress-value">${minGoal.value}</span>
+        <button
+          id="progress-btn"
+          onClick$={() => {
+            minGoal.value += 500;
+          }}
+        >
+          +
+        </button>
+      </div>
+
+      <ProgressRoot min={minGoal.value} />
+      <ProgressRootShowHide min={computedGoal.value} />
+      <ProgressRootPromise min={minGoal.value} />
+    </div>
+  );
+});

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

@@ -295,6 +295,29 @@ test.describe("attributes", () => {
       await expect(button).not.toHaveAttribute("aria-label");
       await expect(button).not.toHaveAttribute("title");
     });
+
+    test("should rerun vnode-diff when QRL is not resolved", async ({
+      page,
+    }) => {
+      const incrementButton = page.locator("#progress-btn");
+      const hideButton = page.locator("#progress-hide");
+      const progress1 = page.locator("#progress-1");
+      const progress2 = page.locator("#progress-2");
+      const progress3 = page.locator("#progress-3");
+
+      await expect(progress1).toHaveAttribute("aria-valuetext", "200000%");
+      await expect(progress2).toHaveAttribute("aria-valuetext", "2100");
+      await expect(progress3).toHaveAttribute("aria-valuetext", "200000%");
+
+      await hideButton.click();
+      await hideButton.click();
+
+      await incrementButton.click();
+
+      await expect(progress1).toHaveAttribute("aria-valuetext", "250000%");
+      await expect(progress2).toHaveAttribute("aria-valuetext", "2600");
+      await expect(progress3).toHaveAttribute("aria-valuetext", "250000%");
+    });
   }
 
   tests();