From fd66aa63822a0304d69ecfabd2af8fda83060fd5 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 30 Jan 2024 12:25:10 -0500 Subject: [PATCH 1/2] Always warn if client component is an async function 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. --- .../react-reconciler/src/ReactFiberHooks.js | 49 ++++------------ .../src/__tests__/ReactUse-test.js | 56 ++++++++++++------- 2 files changed, 46 insertions(+), 59 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 3cdfae52c25db..2ace73aa69ad6 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -388,10 +388,7 @@ function warnOnHookMismatchInDev(currentHookName: HookType): void { } } -function warnIfAsyncClientComponent( - Component: Function, - componentDoesIncludeHooks: boolean, -) { +function warnIfAsyncClientComponent(Component: Function) { if (__DEV__) { // This dev-only check only works for detecting native async functions, // not transpiled ones. There's also a prod check that we use to prevent @@ -402,40 +399,16 @@ function warnIfAsyncClientComponent( // $FlowIgnore[method-unbinding] Object.prototype.toString.call(Component) === '[object AsyncFunction]'; if (isAsyncFunction) { - // Encountered an async Client Component. This is not yet supported, - // except in certain constrained cases, like during a route navigation. + // Encountered an async Client Component. This is not yet supported. const componentName = getComponentNameFromFiber(currentlyRenderingFiber); if (!didWarnAboutAsyncClientComponent.has(componentName)) { didWarnAboutAsyncClientComponent.add(componentName); - - // Check if this is a sync update. We use the "root" render lanes here - // because the "subtree" render lanes may include additional entangled - // lanes related to revealing previously hidden content. - const root = getWorkInProgressRoot(); - const rootRenderLanes = getWorkInProgressRootRenderLanes(); - if (root !== null && includesBlockingLane(root, rootRenderLanes)) { - console.error( - 'async/await is not yet supported in Client Components, only ' + - 'Server Components. This error is often caused by accidentally ' + - "adding `'use client'` to a module that was originally written " + - 'for the server.', - ); - } else { - // This is a concurrent (Transition, Retry, etc) render. We don't - // warn in these cases. - // - // However, Async Components are forbidden to include hooks, even - // during a transition, so let's check for that here. - // - // TODO: Add a corresponding warning to Server Components runtime. - if (componentDoesIncludeHooks) { - console.error( - 'Hooks are not supported inside an async component. This ' + - "error is often caused by accidentally adding `'use client'` " + - 'to a module that was originally written for the server.', - ); - } - } + console.error( + 'async/await is not yet supported in Client Components, only ' + + 'Server Components. This error is often caused by accidentally ' + + "adding `'use client'` to a module that was originally written " + + 'for the server.', + ); } } } @@ -521,6 +494,8 @@ export function renderWithHooks( // Used for hot reloading: ignorePreviousDependencies = current !== null && current.type !== workInProgress.type; + + warnIfAsyncClientComponent(Component); } workInProgress.memoizedState = null; @@ -637,10 +612,6 @@ function finishRenderingHooks( ): void { if (__DEV__) { workInProgress._debugHookTypes = hookTypesDev; - - const componentDoesIncludeHooks = - workInProgressHook !== null || thenableIndexCounter !== 0; - warnIfAsyncClientComponent(Component, componentDoesIncludeHooks); } // We can assume the previous dispatcher is always this one, since we set it diff --git a/packages/react-reconciler/src/__tests__/ReactUse-test.js b/packages/react-reconciler/src/__tests__/ReactUse-test.js index fd49f8c8d784f..059331afb0bec 100644 --- a/packages/react-reconciler/src/__tests__/ReactUse-test.js +++ b/packages/react-reconciler/src/__tests__/ReactUse-test.js @@ -1733,25 +1733,35 @@ describe('ReactUse', () => { ); }); - test('warn if async client component calls a hook (e.g. useState)', async () => { - async function AsyncClientComponent() { - useState(); - return ; - } + test( + 'warn if async client component calls a hook (e.g. useState) ' + + 'during a non-sync update', + async () => { + async function AsyncClientComponent() { + useState(); + return ; + } - const root = ReactNoop.createRoot(); - await expect(async () => { - await act(() => { - startTransition(() => { - root.render(); + const root = ReactNoop.createRoot(); + await expect(async () => { + await act(() => { + startTransition(() => { + root.render(); + }); }); - }); - }).toErrorDev([ - 'Hooks are not supported inside an async component. This ' + - "error is often caused by accidentally adding `'use client'` " + - 'to a module that was originally written for the server.', - ]); - }); + }).toErrorDev([ + // Note: This used to log a different warning about not using hooks + // inside async components, like we do on the server. Since then, we + // decided to warn for _any_ async client component regardless of + // whether the update is sync. But if we ever add back support for async + // client components, we should add back the hook warning. + 'async/await is not yet supported in Client Components, only Server ' + + 'Components. This error is often caused by accidentally adding ' + + "`'use client'` to a module that was originally written for " + + 'the server.', + ]); + }, + ); test('warn if async client component calls a hook (e.g. use)', async () => { const promise = Promise.resolve(); @@ -1769,9 +1779,15 @@ describe('ReactUse', () => { }); }); }).toErrorDev([ - 'Hooks are not supported inside an async component. This ' + - "error is often caused by accidentally adding `'use client'` " + - 'to a module that was originally written for the server.', + // Note: This used to log a different warning about not using hooks + // inside async components, like we do on the server. Since then, we + // decided to warn for _any_ async client component regardless of + // whether the update is sync. But if we ever add back support for async + // client components, we should add back the hook warning. + 'async/await is not yet supported in Client Components, only Server ' + + 'Components. This error is often caused by accidentally adding ' + + "`'use client'` to a module that was originally written for " + + 'the server.', ]); }); }); From 54b16175650535fc99245aaa45925c303ec1647c Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 30 Jan 2024 12:55:20 -0500 Subject: [PATCH 2/2] Warn if uncached promise suspends during render We do not yet support async/await on the client. You can only unwrap a promise that was passed from a Server Component, or a Suspense-compatible framework like Relay. --- .../src/ReactFiberThenable.js | 63 +++++++++++++++++-- .../src/__tests__/ReactUse-test.js | 40 +++++++++--- 2 files changed, 90 insertions(+), 13 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberThenable.js b/packages/react-reconciler/src/ReactFiberThenable.js index 5da965e41c4fb..956f81e89270a 100644 --- a/packages/react-reconciler/src/ReactFiberThenable.js +++ b/packages/react-reconciler/src/ReactFiberThenable.js @@ -19,7 +19,24 @@ import {getWorkInProgressRoot} from './ReactFiberWorkLoop'; import ReactSharedInternals from 'shared/ReactSharedInternals'; const {ReactCurrentActQueue} = ReactSharedInternals; -export opaque type ThenableState = Array>; +opaque type ThenableStateDev = { + didWarnAboutUncachedPromise: boolean, + thenables: Array>, +}; + +opaque type ThenableStateProd = Array>; + +export opaque type ThenableState = ThenableStateDev | ThenableStateProd; + +function getThenablesFromState(state: ThenableState): Array> { + if (__DEV__) { + const devState: ThenableStateDev = (state: any); + return devState.thenables; + } else { + const prodState = (state: any); + return prodState; + } +} // An error that is thrown (e.g. by `use`) to trigger Suspense. If we // detect this is caught by userspace, we'll log a warning in development. @@ -56,7 +73,14 @@ export const noopSuspenseyCommitThenable = { export function createThenableState(): ThenableState { // The ThenableState is created the first time a component suspends. If it // suspends again, we'll reuse the same state. - return []; + if (__DEV__) { + return { + didWarnAboutUncachedPromise: false, + thenables: [], + }; + } else { + return []; + } } export function isThenableResolved(thenable: Thenable): boolean { @@ -74,15 +98,44 @@ export function trackUsedThenable( if (__DEV__ && ReactCurrentActQueue.current !== null) { ReactCurrentActQueue.didUsePromise = true; } - - const previous = thenableState[index]; + const trackedThenables = getThenablesFromState(thenableState); + const previous = trackedThenables[index]; if (previous === undefined) { - thenableState.push(thenable); + trackedThenables.push(thenable); } else { if (previous !== thenable) { // Reuse the previous thenable, and drop the new one. We can assume // they represent the same value, because components are idempotent. + if (__DEV__) { + const thenableStateDev: ThenableStateDev = (thenableState: any); + if (!thenableStateDev.didWarnAboutUncachedPromise) { + // We should only warn the first time an uncached thenable is + // discovered per component, because if there are multiple, the + // subsequent ones are likely derived from the first. + // + // We track this on the thenableState instead of deduping using the + // component name like we usually do, because in the case of a + // promise-as-React-node, the owner component is likely different from + // the parent that's currently being reconciled. We'd have to track + // the owner using state, which we're trying to move away from. Though + // since this is dev-only, maybe that'd be OK. + // + // However, another benefit of doing it this way is we might + // eventually have a thenableState per memo/Forget boundary instead + // of per component, so this would allow us to have more + // granular warnings. + thenableStateDev.didWarnAboutUncachedPromise = true; + + // TODO: This warning should link to a corresponding docs page. + console.error( + 'A component was suspended by an uncached promise. Creating ' + + 'promises inside a Client Component or hook is not yet ' + + 'supported, except via a Suspense-compatible library or framework.', + ); + } + } + // Avoid an unhandled rejection errors for the Promises that we'll // intentionally ignore. thenable.then(noop, noop); diff --git a/packages/react-reconciler/src/__tests__/ReactUse-test.js b/packages/react-reconciler/src/__tests__/ReactUse-test.js index 059331afb0bec..12e93620b5ded 100644 --- a/packages/react-reconciler/src/__tests__/ReactUse-test.js +++ b/packages/react-reconciler/src/__tests__/ReactUse-test.js @@ -230,11 +230,17 @@ describe('ReactUse', () => { } const root = ReactNoop.createRoot(); - await act(() => { - startTransition(() => { - root.render(); + await expect(async () => { + await act(() => { + startTransition(() => { + root.render(); + }); }); - }); + }).toErrorDev([ + 'A component was suspended by an uncached promise. Creating ' + + 'promises inside a Client Component or hook is not yet ' + + 'supported, except via a Suspense-compatible library or framework.', + ]); assertLog(['ABC']); expect(root).toMatchRenderedOutput('ABC'); }); @@ -394,11 +400,20 @@ describe('ReactUse', () => { } const root = ReactNoop.createRoot(); - await act(() => { - startTransition(() => { - root.render(); + await expect(async () => { + await act(() => { + startTransition(() => { + root.render(); + }); }); - }); + }).toErrorDev([ + 'A component was suspended by an uncached promise. Creating ' + + 'promises inside a Client Component or hook is not yet ' + + 'supported, except via a Suspense-compatible library or framework.', + 'A component was suspended by an uncached promise. Creating ' + + 'promises inside a Client Component or hook is not yet ' + + 'supported, except via a Suspense-compatible library or framework.', + ]); assertLog([ // First attempt. The uncached promise suspends. 'Suspend! [Async]', @@ -1759,6 +1774,9 @@ describe('ReactUse', () => { 'Components. This error is often caused by accidentally adding ' + "`'use client'` to a module that was originally written for " + 'the server.', + 'A component was suspended by an uncached promise. Creating ' + + 'promises inside a Client Component or hook is not yet ' + + 'supported, except via a Suspense-compatible library or framework.', ]); }, ); @@ -1788,6 +1806,12 @@ describe('ReactUse', () => { 'Components. This error is often caused by accidentally adding ' + "`'use client'` to a module that was originally written for " + 'the server.', + 'A component was suspended by an uncached promise. Creating ' + + 'promises inside a Client Component or hook is not yet ' + + 'supported, except via a Suspense-compatible library or framework.', + 'A component was suspended by an uncached promise. Creating ' + + 'promises inside a Client Component or hook is not yet ' + + 'supported, except via a Suspense-compatible library or framework.', ]); }); });