Skip to content

Commit cd87816

Browse files
acdlitegaearon
acdlite
authored and
gaearon
committed
Always warn if client component suspends with an uncached promise (#28159)
Previously we only warned during a synchronous update, because we eventually want to support async client components in controlled scenarios, like during navigations. However, we're going to warn in all cases for now until we figure out how that should work.
1 parent c74292f commit cd87816

3 files changed

Lines changed: 136 additions & 72 deletions

File tree

‎packages/react-reconciler/src/ReactFiberHooks.js‎

Lines changed: 10 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -388,10 +388,7 @@ function warnOnHookMismatchInDev(currentHookName: HookType): void {
388388
}
389389
}
390390

391-
function warnIfAsyncClientComponent(
392-
Component: Function,
393-
componentDoesIncludeHooks: boolean,
394-
) {
391+
function warnIfAsyncClientComponent(Component: Function) {
395392
if (__DEV__) {
396393
// This dev-only check only works for detecting native async functions,
397394
// not transpiled ones. There's also a prod check that we use to prevent
@@ -402,40 +399,16 @@ function warnIfAsyncClientComponent(
402399
// $FlowIgnore[method-unbinding]
403400
Object.prototype.toString.call(Component) === '[object AsyncFunction]';
404401
if (isAsyncFunction) {
405-
// Encountered an async Client Component. This is not yet supported,
406-
// except in certain constrained cases, like during a route navigation.
402+
// Encountered an async Client Component. This is not yet supported.
407403
const componentName = getComponentNameFromFiber(currentlyRenderingFiber);
408404
if (!didWarnAboutAsyncClientComponent.has(componentName)) {
409405
didWarnAboutAsyncClientComponent.add(componentName);
410-
411-
// Check if this is a sync update. We use the "root" render lanes here
412-
// because the "subtree" render lanes may include additional entangled
413-
// lanes related to revealing previously hidden content.
414-
const root = getWorkInProgressRoot();
415-
const rootRenderLanes = getWorkInProgressRootRenderLanes();
416-
if (root !== null && includesBlockingLane(root, rootRenderLanes)) {
417-
console.error(
418-
'async/await is not yet supported in Client Components, only ' +
419-
'Server Components. This error is often caused by accidentally ' +
420-
"adding `'use client'` to a module that was originally written " +
421-
'for the server.',
422-
);
423-
} else {
424-
// This is a concurrent (Transition, Retry, etc) render. We don't
425-
// warn in these cases.
426-
//
427-
// However, Async Components are forbidden to include hooks, even
428-
// during a transition, so let's check for that here.
429-
//
430-
// TODO: Add a corresponding warning to Server Components runtime.
431-
if (componentDoesIncludeHooks) {
432-
console.error(
433-
'Hooks are not supported inside an async component. This ' +
434-
"error is often caused by accidentally adding `'use client'` " +
435-
'to a module that was originally written for the server.',
436-
);
437-
}
438-
}
406+
console.error(
407+
'async/await is not yet supported in Client Components, only ' +
408+
'Server Components. This error is often caused by accidentally ' +
409+
"adding `'use client'` to a module that was originally written " +
410+
'for the server.',
411+
);
439412
}
440413
}
441414
}
@@ -521,6 +494,8 @@ export function renderWithHooks<Props, SecondArg>(
521494
// Used for hot reloading:
522495
ignorePreviousDependencies =
523496
current !== null && current.type !== workInProgress.type;
497+
498+
warnIfAsyncClientComponent(Component);
524499
}
525500

526501
workInProgress.memoizedState = null;
@@ -637,10 +612,6 @@ function finishRenderingHooks<Props, SecondArg>(
637612
): void {
638613
if (__DEV__) {
639614
workInProgress._debugHookTypes = hookTypesDev;
640-
641-
const componentDoesIncludeHooks =
642-
workInProgressHook !== null || thenableIndexCounter !== 0;
643-
warnIfAsyncClientComponent(Component, componentDoesIncludeHooks);
644615
}
645616

646617
// We can assume the previous dispatcher is always this one, since we set it

‎packages/react-reconciler/src/ReactFiberThenable.js‎

Lines changed: 58 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,24 @@ import {getWorkInProgressRoot} from './ReactFiberWorkLoop';
1919
import ReactSharedInternals from 'shared/ReactSharedInternals';
2020
const {ReactCurrentActQueue} = ReactSharedInternals;
2121

22-
export opaque type ThenableState = Array<Thenable<any>>;
22+
opaque type ThenableStateDev = {
23+
didWarnAboutUncachedPromise: boolean,
24+
thenables: Array<Thenable<any>>,
25+
};
26+
27+
opaque type ThenableStateProd = Array<Thenable<any>>;
28+
29+
export opaque type ThenableState = ThenableStateDev | ThenableStateProd;
30+
31+
function getThenablesFromState(state: ThenableState): Array<Thenable<any>> {
32+
if (__DEV__) {
33+
const devState: ThenableStateDev = (state: any);
34+
return devState.thenables;
35+
} else {
36+
const prodState = (state: any);
37+
return prodState;
38+
}
39+
}
2340

2441
// An error that is thrown (e.g. by `use`) to trigger Suspense. If we
2542
// detect this is caught by userspace, we'll log a warning in development.
@@ -56,7 +73,14 @@ export const noopSuspenseyCommitThenable = {
5673
export function createThenableState(): ThenableState {
5774
// The ThenableState is created the first time a component suspends. If it
5875
// suspends again, we'll reuse the same state.
59-
return [];
76+
if (__DEV__) {
77+
return {
78+
didWarnAboutUncachedPromise: false,
79+
thenables: [],
80+
};
81+
} else {
82+
return [];
83+
}
6084
}
6185

6286
export function isThenableResolved(thenable: Thenable<mixed>): boolean {
@@ -74,15 +98,44 @@ export function trackUsedThenable<T>(
7498
if (__DEV__ && ReactCurrentActQueue.current !== null) {
7599
ReactCurrentActQueue.didUsePromise = true;
76100
}
77-
78-
const previous = thenableState[index];
101+
const trackedThenables = getThenablesFromState(thenableState);
102+
const previous = trackedThenables[index];
79103
if (previous === undefined) {
80-
thenableState.push(thenable);
104+
trackedThenables.push(thenable);
81105
} else {
82106
if (previous !== thenable) {
83107
// Reuse the previous thenable, and drop the new one. We can assume
84108
// they represent the same value, because components are idempotent.
85109

110+
if (__DEV__) {
111+
const thenableStateDev: ThenableStateDev = (thenableState: any);
112+
if (!thenableStateDev.didWarnAboutUncachedPromise) {
113+
// We should only warn the first time an uncached thenable is
114+
// discovered per component, because if there are multiple, the
115+
// subsequent ones are likely derived from the first.
116+
//
117+
// We track this on the thenableState instead of deduping using the
118+
// component name like we usually do, because in the case of a
119+
// promise-as-React-node, the owner component is likely different from
120+
// the parent that's currently being reconciled. We'd have to track
121+
// the owner using state, which we're trying to move away from. Though
122+
// since this is dev-only, maybe that'd be OK.
123+
//
124+
// However, another benefit of doing it this way is we might
125+
// eventually have a thenableState per memo/Forget boundary instead
126+
// of per component, so this would allow us to have more
127+
// granular warnings.
128+
thenableStateDev.didWarnAboutUncachedPromise = true;
129+
130+
// TODO: This warning should link to a corresponding docs page.
131+
console.error(
132+
'A component was suspended by an uncached promise. Creating ' +
133+
'promises inside a Client Component or hook is not yet ' +
134+
'supported, except via a Suspense-compatible library or framework.',
135+
);
136+
}
137+
}
138+
86139
// Avoid an unhandled rejection errors for the Promises that we'll
87140
// intentionally ignore.
88141
thenable.then(noop, noop);

‎packages/react-reconciler/src/__tests__/ReactUse-test.js‎

Lines changed: 68 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -230,11 +230,17 @@ describe('ReactUse', () => {
230230
}
231231

232232
const root = ReactNoop.createRoot();
233-
await act(() => {
234-
startTransition(() => {
235-
root.render(<App />);
233+
await expect(async () => {
234+
await act(() => {
235+
startTransition(() => {
236+
root.render(<App />);
237+
});
236238
});
237-
});
239+
}).toErrorDev([
240+
'A component was suspended by an uncached promise. Creating ' +
241+
'promises inside a Client Component or hook is not yet ' +
242+
'supported, except via a Suspense-compatible library or framework.',
243+
]);
238244
assertLog(['ABC']);
239245
expect(root).toMatchRenderedOutput('ABC');
240246
});
@@ -394,11 +400,20 @@ describe('ReactUse', () => {
394400
}
395401

396402
const root = ReactNoop.createRoot();
397-
await act(() => {
398-
startTransition(() => {
399-
root.render(<App />);
403+
await expect(async () => {
404+
await act(() => {
405+
startTransition(() => {
406+
root.render(<App />);
407+
});
400408
});
401-
});
409+
}).toErrorDev([
410+
'A component was suspended by an uncached promise. Creating ' +
411+
'promises inside a Client Component or hook is not yet ' +
412+
'supported, except via a Suspense-compatible library or framework.',
413+
'A component was suspended by an uncached promise. Creating ' +
414+
'promises inside a Client Component or hook is not yet ' +
415+
'supported, except via a Suspense-compatible library or framework.',
416+
]);
402417
assertLog([
403418
// First attempt. The uncached promise suspends.
404419
'Suspend! [Async]',
@@ -1733,25 +1748,38 @@ describe('ReactUse', () => {
17331748
);
17341749
});
17351750

1736-
test('warn if async client component calls a hook (e.g. useState)', async () => {
1737-
async function AsyncClientComponent() {
1738-
useState();
1739-
return <Text text="Hi" />;
1740-
}
1751+
test(
1752+
'warn if async client component calls a hook (e.g. useState) ' +
1753+
'during a non-sync update',
1754+
async () => {
1755+
async function AsyncClientComponent() {
1756+
useState();
1757+
return <Text text="Hi" />;
1758+
}
17411759

1742-
const root = ReactNoop.createRoot();
1743-
await expect(async () => {
1744-
await act(() => {
1745-
startTransition(() => {
1746-
root.render(<AsyncClientComponent />);
1760+
const root = ReactNoop.createRoot();
1761+
await expect(async () => {
1762+
await act(() => {
1763+
startTransition(() => {
1764+
root.render(<AsyncClientComponent />);
1765+
});
17471766
});
1748-
});
1749-
}).toErrorDev([
1750-
'Hooks are not supported inside an async component. This ' +
1751-
"error is often caused by accidentally adding `'use client'` " +
1752-
'to a module that was originally written for the server.',
1753-
]);
1754-
});
1767+
}).toErrorDev([
1768+
// Note: This used to log a different warning about not using hooks
1769+
// inside async components, like we do on the server. Since then, we
1770+
// decided to warn for _any_ async client component regardless of
1771+
// whether the update is sync. But if we ever add back support for async
1772+
// client components, we should add back the hook warning.
1773+
'async/await is not yet supported in Client Components, only Server ' +
1774+
'Components. This error is often caused by accidentally adding ' +
1775+
"`'use client'` to a module that was originally written for " +
1776+
'the server.',
1777+
'A component was suspended by an uncached promise. Creating ' +
1778+
'promises inside a Client Component or hook is not yet ' +
1779+
'supported, except via a Suspense-compatible library or framework.',
1780+
]);
1781+
},
1782+
);
17551783

17561784
test('warn if async client component calls a hook (e.g. use)', async () => {
17571785
const promise = Promise.resolve();
@@ -1769,9 +1797,21 @@ describe('ReactUse', () => {
17691797
});
17701798
});
17711799
}).toErrorDev([
1772-
'Hooks are not supported inside an async component. This ' +
1773-
"error is often caused by accidentally adding `'use client'` " +
1774-
'to a module that was originally written for the server.',
1800+
// Note: This used to log a different warning about not using hooks
1801+
// inside async components, like we do on the server. Since then, we
1802+
// decided to warn for _any_ async client component regardless of
1803+
// whether the update is sync. But if we ever add back support for async
1804+
// client components, we should add back the hook warning.
1805+
'async/await is not yet supported in Client Components, only Server ' +
1806+
'Components. This error is often caused by accidentally adding ' +
1807+
"`'use client'` to a module that was originally written for " +
1808+
'the server.',
1809+
'A component was suspended by an uncached promise. Creating ' +
1810+
'promises inside a Client Component or hook is not yet ' +
1811+
'supported, except via a Suspense-compatible library or framework.',
1812+
'A component was suspended by an uncached promise. Creating ' +
1813+
'promises inside a Client Component or hook is not yet ' +
1814+
'supported, except via a Suspense-compatible library or framework.',
17751815
]);
17761816
});
17771817
});

0 commit comments

Comments
 (0)