From 0a97d07802d1fc5c98b32a74bcc7d94dd3e61b36 Mon Sep 17 00:00:00 2001 From: Shu Ding Date: Sat, 22 Oct 2022 22:13:34 -0700 Subject: [PATCH] Improve error messages (#41669) Same as #41636 but with a new condition. ## Bug - [ ] Related issues linked using `fixes #number` - [ ] Integration tests added - [ ] Errors have a helpful link attached, see `contributing.md` ## Feature - [ ] Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR. - [ ] Related issues linked using `fixes #number` - [ ] Integration tests added - [ ] Documentation added - [ ] Telemetry added. In case of a feature if it's used or not. - [ ] Errors have a helpful link attached, see `contributing.md` ## Documentation / Examples - [ ] Make sure the linting passes by running `pnpm lint` - [ ] The "examples guidelines" are followed from [our contributing doc](https://github.com/vercel/next.js/blob/canary/contributing/examples/adding-examples.md) Co-authored-by: Jiachi Liu --- .../core/src/react_server_components.rs | 9 +--- .../wellknown-errors-plugin/parseRSC.ts | 22 +++++--- .../app/app/hooks/use-cookies/client/page.js | 10 ---- .../app/app/hooks/use-headers/client/page.js | 10 ---- .../app/hooks/use-preview-data/client/page.js | 10 ---- .../app/app/old-router/client-router.js | 17 ------ .../e2e/app-dir/app/app/old-router/is-null.js | 7 --- test/e2e/app-dir/app/app/old-router/page.js | 12 ----- test/e2e/app-dir/app/app/old-router/router.js | 19 ------- .../app/app/old-router/server-router.js | 15 ------ test/e2e/app-dir/index.test.ts | 53 ------------------- 11 files changed, 16 insertions(+), 168 deletions(-) delete mode 100644 test/e2e/app-dir/app/app/hooks/use-cookies/client/page.js delete mode 100644 test/e2e/app-dir/app/app/hooks/use-headers/client/page.js delete mode 100644 test/e2e/app-dir/app/app/hooks/use-preview-data/client/page.js delete mode 100644 test/e2e/app-dir/app/app/old-router/client-router.js delete mode 100644 test/e2e/app-dir/app/app/old-router/is-null.js delete mode 100644 test/e2e/app-dir/app/app/old-router/page.js delete mode 100644 test/e2e/app-dir/app/app/old-router/router.js delete mode 100644 test/e2e/app-dir/app/app/old-router/server-router.js diff --git a/packages/next-swc/crates/core/src/react_server_components.rs b/packages/next-swc/crates/core/src/react_server_components.rs index 7527d43cefb3d..a5a1f4f431fb0 100644 --- a/packages/next-swc/crates/core/src/react_server_components.rs +++ b/packages/next-swc/crates/core/src/react_server_components.rs @@ -395,14 +395,9 @@ pub fn server_components( JsWord::from("client-only"), JsWord::from("react-dom/client"), JsWord::from("react-dom/server"), - // TODO-APP: JsWord::from("next/router"), - // TODO-APP: Rule out client hooks. - ], - invalid_client_imports: vec![ - JsWord::from("server-only"), - // TODO-APP: Rule out server hooks such as `useCookies`, `useHeaders`, - // `usePreviewData`. + JsWord::from("next/router"), ], + invalid_client_imports: vec![JsWord::from("server-only"), JsWord::from("next/headers")], invalid_server_react_dom_apis: vec![ JsWord::from("findDOMNode"), JsWord::from("flushSync"), diff --git a/packages/next/build/webpack/plugins/wellknown-errors-plugin/parseRSC.ts b/packages/next/build/webpack/plugins/wellknown-errors-plugin/parseRSC.ts index 7995032ac9726..0c78ff1d82f6d 100644 --- a/packages/next/build/webpack/plugins/wellknown-errors-plugin/parseRSC.ts +++ b/packages/next/build/webpack/plugins/wellknown-errors-plugin/parseRSC.ts @@ -25,14 +25,20 @@ export function formatRSCErrorMessage( '\n\nMaybe one of these should be marked as a client entry with "use client":\n' } else if (NEXT_RSC_ERR_SERVER_IMPORT.test(message)) { const matches = message.match(NEXT_RSC_ERR_SERVER_IMPORT) - if (matches && matches[1] === 'react-dom/server') { - // If importing "react-dom/server", we should show a different error. - formattedMessage = `\n\nYou're importing a component that imports react-dom/server. To fix it, render or return the content directly as a Server Component instead for perf and security.` - } else { - formattedMessage = message.replace( - NEXT_RSC_ERR_SERVER_IMPORT, - `\n\nYou're importing a component that imports $1. It only works in a Client Component but none of its parents are marked with "use client", so they're Server Components by default.\n\n` - ) + switch (matches && matches[1]) { + case 'react-dom/server': + // If importing "react-dom/server", we should show a different error. + formattedMessage = `\n\nYou're importing a component that imports react-dom/server. To fix it, render or return the content directly as a Server Component instead for perf and security.` + break + case 'next/router': + // If importing "next/router", we should tell them to use "next/navigation". + formattedMessage = `\n\nYou have a Server Component that imports next/router. Use next/navigation instead.` + break + default: + formattedMessage = message.replace( + NEXT_RSC_ERR_SERVER_IMPORT, + `\n\nYou're importing a component that imports $1. It only works in a Client Component but none of its parents are marked with "use client", so they're Server Components by default.\n\n` + ) } formattedVerboseMessage = '\n\nMaybe one of these should be marked as a client entry "use client":\n' diff --git a/test/e2e/app-dir/app/app/hooks/use-cookies/client/page.js b/test/e2e/app-dir/app/app/hooks/use-cookies/client/page.js deleted file mode 100644 index 86cc3c99f86fa..0000000000000 --- a/test/e2e/app-dir/app/app/hooks/use-cookies/client/page.js +++ /dev/null @@ -1,10 +0,0 @@ -'use client' - -import { cookies } from 'next/headers' - -export default function Page() { - // This should throw an error. - cookies() - - return null -} diff --git a/test/e2e/app-dir/app/app/hooks/use-headers/client/page.js b/test/e2e/app-dir/app/app/hooks/use-headers/client/page.js deleted file mode 100644 index 70d2a68fdb81e..0000000000000 --- a/test/e2e/app-dir/app/app/hooks/use-headers/client/page.js +++ /dev/null @@ -1,10 +0,0 @@ -'use client' - -import { headers } from 'next/headers' - -export default function Page() { - // This should throw an error. - headers() - - return null -} diff --git a/test/e2e/app-dir/app/app/hooks/use-preview-data/client/page.js b/test/e2e/app-dir/app/app/hooks/use-preview-data/client/page.js deleted file mode 100644 index 3a5f43da9db47..0000000000000 --- a/test/e2e/app-dir/app/app/hooks/use-preview-data/client/page.js +++ /dev/null @@ -1,10 +0,0 @@ -'use client' - -import { previewData } from 'next/headers' - -export default function Page() { - // This should throw an error. - previewData() - - return null -} diff --git a/test/e2e/app-dir/app/app/old-router/client-router.js b/test/e2e/app-dir/app/app/old-router/client-router.js deleted file mode 100644 index f9b0e84a71cba..0000000000000 --- a/test/e2e/app-dir/app/app/old-router/client-router.js +++ /dev/null @@ -1,17 +0,0 @@ -'use client' - -import { useRouter, withRouter } from 'next/router' -import IsNull from './is-null' - -function ClientRouter({ router: withRouter }) { - const router = useRouter() - - return ( - <> - - - - ) -} - -export default withRouter(ClientRouter) diff --git a/test/e2e/app-dir/app/app/old-router/is-null.js b/test/e2e/app-dir/app/app/old-router/is-null.js deleted file mode 100644 index 3876fbe873654..0000000000000 --- a/test/e2e/app-dir/app/app/old-router/is-null.js +++ /dev/null @@ -1,7 +0,0 @@ -export default function IsNull({ value }) { - if (value === null) { - return
Value Was Null
- } - - return
Value Was Not Null
-} diff --git a/test/e2e/app-dir/app/app/old-router/page.js b/test/e2e/app-dir/app/app/old-router/page.js deleted file mode 100644 index 19b3e3a835627..0000000000000 --- a/test/e2e/app-dir/app/app/old-router/page.js +++ /dev/null @@ -1,12 +0,0 @@ -import { cookies } from 'next/headers' -import Router from './router' - -export default function Page() { - cookies() - - return ( -
- -
- ) -} diff --git a/test/e2e/app-dir/app/app/old-router/router.js b/test/e2e/app-dir/app/app/old-router/router.js deleted file mode 100644 index 7cddbe4181dc8..0000000000000 --- a/test/e2e/app-dir/app/app/old-router/router.js +++ /dev/null @@ -1,19 +0,0 @@ -import { useRouter, withRouter } from 'next/router' -import IsNull from './is-null' -import ServerRouter from './server-router' -import ClientRouter from './client-router' - -function SharedRouter({ router: withRouter }) { - const router = useRouter() - - return ( - <> - - - - - - ) -} - -export default withRouter(SharedRouter) diff --git a/test/e2e/app-dir/app/app/old-router/server-router.js b/test/e2e/app-dir/app/app/old-router/server-router.js deleted file mode 100644 index 9e1291329c497..0000000000000 --- a/test/e2e/app-dir/app/app/old-router/server-router.js +++ /dev/null @@ -1,15 +0,0 @@ -import { useRouter, withRouter } from 'next/router' -import IsNull from './is-null' - -function ServerRouter({ router: withRouter }) { - const router = useRouter() - - return ( - <> - - - - ) -} - -export default withRouter(ServerRouter) diff --git a/test/e2e/app-dir/index.test.ts b/test/e2e/app-dir/index.test.ts index 53987c63fc717..13380335692b8 100644 --- a/test/e2e/app-dir/index.test.ts +++ b/test/e2e/app-dir/index.test.ts @@ -893,23 +893,6 @@ describe('app dir', () => { }) describe('next/router', () => { - // `useRouter` should not be accessible in server components. - it('should always return null when accessed from /app', async () => { - const browser = await webdriver(next.url, '/old-router') - - try { - await browser.waitForElementByCss('#old-router') - - const notNull = await browser.elementsByCss('.was-not-null') - expect(notNull.length).toBe(0) - - const wasNull = await browser.elementsByCss('.was-null') - expect(wasNull.length).toBe(6) - } finally { - await browser.close() - } - }) - it('should support router.back and router.forward', async () => { const browser = await webdriver(next.url, '/back-forward/1') @@ -1134,42 +1117,6 @@ describe('app dir', () => { describe('client components', () => { describe('hooks', () => { - describe('cookies function', () => { - // TODO-APP: should enable when implemented - it.skip('should throw an error when imported', async () => { - const res = await fetchViaHTTP( - next.url, - '/hooks/use-cookies/client' - ) - expect(res.status).toBe(500) - expect(await res.text()).toContain('Internal Server Error') - }) - }) - - describe('previewData function', () => { - // TODO-APP: should enable when implemented - it.skip('should throw an error when imported', async () => { - const res = await fetchViaHTTP( - next.url, - '/hooks/use-preview-data/client' - ) - expect(res.status).toBe(500) - expect(await res.text()).toContain('Internal Server Error') - }) - }) - - describe('headers function', () => { - // TODO-APP: should enable when implemented - it.skip('should throw an error when imported', async () => { - const res = await fetchViaHTTP( - next.url, - '/hooks/use-headers/client' - ) - expect(res.status).toBe(500) - expect(await res.text()).toContain('Internal Server Error') - }) - }) - describe('usePathname', () => { it('should have the correct pathname', async () => { const html = await renderViaHTTP(next.url, '/hooks/use-pathname')