Skip to content

Commit

Permalink
refactor: rewrite config schema in zod (#56383)
Browse files Browse the repository at this point in the history
The PR supersedes the #53150, which is way too outdated, has way too many conflicts, and also heavily relies on GitHub Copilot (which makes the progress slow and tedious).

The PR uses [`json-schema-to-zod`](https://github.com/StefanTerdell/json-schema-to-zod) (instead of the GitHub Copilot) to generate the zod schema, and manually replaces all generated `z.customRefine` with my hand-written zod schema.

TODO:

- [x] Convert schema
- [x] Reduce `z.any()` usage
- [x] Create human-readable errors from the `ZodError`
- [x] Update test cases to reflect the latest error message

-----

The benefit of using zod over ajv:

- Easier maintenance: zod schema is straightforward to compose.
- Better typescript support: config schema now strictly reflects the `NextConfig` type.
- Smaller installation size: by replacing `ajv` and `@segment/ajv-human-errors` w/ `zod`, I am able to reduce installation size by 114 KiB.
- Better Extension: the zod error message is easy to customize.

-----

In the previous PR #56083, @feedthejim replaces `zod` w/ `superstruct`. `superstruct` is lightweight and fast, which makes it perfect for creating simple schemas for RSC payload. But, this also means `superstruct` has its limitations compared to `zod`:

- `superstruct`'s syntax is different, and some utilities's usage is counter-intuitive:
  - `z.array(z.string()).gt(1)` vs `s.size(s.array(s.string()), 1)`
  - `z.numer().gt(1)` vs `s.size(s.number(), 1)`, `s.min(s.number(), 1)`
  - `z.boolean().optional().nullable()` vs `s.nullable(s.optional(z.boolean()))`
- `superstruct` has weaker TypeScript support and worse DX compared to `zod` when composing huge schema:
  - `zod.ZodType + z.object()` can provide a more detailed type mismatch message on which specific property is the culprit, while `Describe + s.object()` provides almost no information at all.
- `zod`'s schema is more powerful
  - `z.function()` supports `z.args()` and `z.returns()`, while `superstruct` only has `s.func()`
  - zod also has Promise type `z.promise()` and intersection type `z.and()`
- `superstruct`'s error is harder to parse compared to `zod`'s `ZodError`

So in the PR, I re-introduced `zod` for `next.config.js` validation.
  • Loading branch information
SukkaW authored and huozhi committed Oct 5, 2023
1 parent 35e4539 commit 72cc026
Show file tree
Hide file tree
Showing 11 changed files with 138 additions and 60 deletions.
19 changes: 19 additions & 0 deletions packages/next-swc/crates/core/src/react_server_components.rs
Original file line number Diff line number Diff line change
Expand Up @@ -344,6 +344,10 @@ impl<C: Comments> ReactServerComponents<C> {
}

fn assert_server_graph(&self, imports: &[ModuleImports], module: &Module) {
// If the
if self.is_from_node_modules(&self.filepath) {
return;
}
for import in imports {
let source = import.source.0.clone();
if self.invalid_server_imports.contains(&source) {
Expand Down Expand Up @@ -391,6 +395,9 @@ impl<C: Comments> ReactServerComponents<C> {
}

fn assert_server_filename(&self, module: &Module) {
if self.is_from_node_modules(&self.filepath) {
return;
}
let is_error_file = Regex::new(r"[\\/]error\.(ts|js)x?$")
.unwrap()
.is_match(&self.filepath);
Expand All @@ -416,6 +423,9 @@ impl<C: Comments> ReactServerComponents<C> {
}

fn assert_client_graph(&self, imports: &[ModuleImports]) {
if self.is_from_node_modules(&self.filepath) {
return;
}
for import in imports {
let source = import.source.0.clone();
if self.invalid_client_imports.contains(&source) {
Expand All @@ -432,6 +442,9 @@ impl<C: Comments> ReactServerComponents<C> {
}

fn assert_invalid_api(&self, module: &Module, is_client_entry: bool) {
if self.is_from_node_modules(&self.filepath) {
return;
}
let is_layout_or_page = Regex::new(r"[\\/](page|layout)\.(ts|js)x?$")
.unwrap()
.is_match(&self.filepath);
Expand Down Expand Up @@ -562,6 +575,12 @@ impl<C: Comments> ReactServerComponents<C> {
},
);
}

fn is_from_node_modules(&self, filepath: &str) -> bool {
Regex::new(r"[\\/]node_modules[\\/]")
.unwrap()
.is_match(filepath)
}
}

pub fn server_components<C: Comments>(
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
'use client'
export default function page() {
return 'page'
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
export const metadata = {
title: 'Next.js',
description: 'Generated by Next.js',
}

export default function RootLayout({ children }) {
return (
<html lang="en">
<body>{children}</body>
</html>
)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default function page() {
return 'page'
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import { useState } from 'react'

export function callClientApi() {
return useState(0)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"name": "client-package",
"exports": "./index.js"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
'use client'

import { cookies } from 'next/headers'

export function callServerApi() {
return cookies()
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"name": "server-package",
"exports": "./index.js"
}
59 changes: 0 additions & 59 deletions test/development/acceptance-app/rsc-build-errors.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -379,63 +379,4 @@ describe('Error overlay - RSC build errors', () => {

await cleanup()
})

it('should show which import caused an error in node_modules', async () => {
const { session, cleanup } = await sandbox(
next,
new Map([
[
'node_modules/client-package/module2.js',
"import { useState } from 'react'",
],
['node_modules/client-package/module1.js', "import './module2.js'"],
['node_modules/client-package/index.js', "import './module1.js'"],
[
'node_modules/client-package/package.json',
outdent`
{
"name": "client-package",
"version": "0.0.1"
}
`,
],
['app/Component.js', "import 'client-package'"],
[
'app/page.js',
outdent`
import './Component.js'
export default function Page() {
return <p>Hello world</p>
}
`,
],
])
)

expect(await session.hasRedbox(true)).toBe(true)
expect(
next.normalizeTestDirContent(await session.getRedboxSource())
).toMatchInlineSnapshot(
next.normalizeSnapshot(`
"./app/Component.js
ReactServerComponentsError:
You're importing a component that needs useState. It only works in a Client Component but none of its parents are marked with \\"use client\\", so they're Server Components by default.
Learn more: https://nextjs.org/docs/getting-started/react-essentials
,-[TEST_DIR/node_modules/client-package/module2.js:1:1]
1 | import { useState } from 'react'
: ^^^^^^^^
\`----
The error was caused by importing 'client-package/index.js' in './app/Component.js'.
Maybe one of these should be marked as a client entry with \\"use client\\":
./app/Component.js
./app/page.js"
`)
)

await cleanup()
})
})
80 changes: 80 additions & 0 deletions test/development/acceptance-app/rsc-runtime-errors.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
import path from 'path'
import { outdent } from 'outdent'
import { FileRef, createNextDescribe } from 'e2e-utils'
import {
check,
getRedboxDescription,
hasRedbox,
shouldRunTurboDevTest,
} from 'next-test-utils'

createNextDescribe(
'Error overlay - RSC runtime errors',
{
files: new FileRef(path.join(__dirname, 'fixtures', 'rsc-runtime-errors')),
packageJson: {
scripts: {
setup: 'cp -r ./node_modules_bak/* ./node_modules',
build: 'yarn setup && next build',
dev: `yarn setup && next ${
shouldRunTurboDevTest() ? 'dev --turbo' : 'dev'
}`,
start: 'next start',
},
},
installCommand: 'yarn',
startCommand: (global as any).isNextDev ? 'yarn dev' : 'yarn start',
},
({ next }) => {
it('should show runtime errors if invalid client API from node_modules is executed', async () => {
await next.patchFile(
'app/server/page.js',
outdent`
import { callClientApi } from 'client-package'
export default function Page() {
callClientApi()
return 'page'
}
`
)

const browser = await next.browser('/server')

await check(
async () => ((await hasRedbox(browser, true)) ? 'success' : 'fail'),
/success/
)
const errorDescription = await getRedboxDescription(browser)

expect(errorDescription).toContain(
`Error: useState only works in Client Components. Add the "use client" directive at the top of the file to use it. Read more: https://nextjs.org/docs/messages/react-client-hook-in-server-component`
)
})

it('should show runtime errors if invalid server API from node_modules is executed', async () => {
await next.patchFile(
'app/client/page.js',
outdent`
'use client'
import { callServerApi } from 'server-package'
export default function Page() {
callServerApi()
return 'page'
}
`
)

const browser = await next.browser('/client')

await check(
async () => ((await hasRedbox(browser, true)) ? 'success' : 'fail'),
/success/
)
const errorDescription = await getRedboxDescription(browser)

expect(errorDescription).toContain(
`Error: Invariant: cookies() expects to have requestAsyncStorage, none available.`
)
})
}
)

0 comments on commit 72cc026

Please sign in to comment.