Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: retry vnode diffing on promise throw #7259

Merged
merged 2 commits into from
Jan 17, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/slimy-weeks-hope.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@qwik.dev/core': patch
---

fix: retry vnode diffing on promise throw
16 changes: 10 additions & 6 deletions packages/qwik/src/core/shared/scheduler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down
21 changes: 17 additions & 4 deletions packages/qwik/src/core/shared/utils/promises.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
16 changes: 16 additions & 0 deletions packages/qwik/src/core/signal/signal-subscriber.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down
21 changes: 20 additions & 1 deletion packages/qwik/src/core/tests/render-promise.spec.tsx
Original file line number Diff line number Diff line change
@@ -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';

Expand All @@ -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');
});
});
28 changes: 0 additions & 28 deletions starters/apps/e2e/src/components/attributes/attributes.e2e.tsx

This file was deleted.

96 changes: 95 additions & 1 deletion starters/apps/e2e/src/components/attributes/attributes.tsx
Original file line number Diff line number Diff line change
@@ -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);
Expand All @@ -13,6 +19,7 @@ export const Attributes = component$(() => {
Rerender
</button>
<AttributesChild v={render.value} key={render.value} />
<ProgressParent />
</>
);
});
Expand Down Expand Up @@ -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>
);
});
23 changes: 23 additions & 0 deletions starters/e2e/e2e.attributes.e2e.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
Loading