-
Notifications
You must be signed in to change notification settings - Fork 29.9k
Fix stale dev types causing build failure after route deletion #86489
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
base: canary
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,58 @@ | ||
| import path from 'path' | ||
|
|
||
| /** | ||
| * Gets the glob patterns for type definition directories in tsconfig. | ||
| * When isolatedDevBuild is enabled, Next.js uses different distDir paths: | ||
| * - Development: "{distDir}/dev" | ||
| * - Production: "{distDir}" | ||
| */ | ||
| export function getTypeDefinitionGlobPatterns( | ||
| distDir: string, | ||
| isolatedDevBuild: boolean | ||
| ): string[] { | ||
| const distDirPosix = | ||
| path.win32.sep === path.sep | ||
| ? distDir.replaceAll(path.win32.sep, path.posix.sep) | ||
| : distDir | ||
|
|
||
| const typeGlobPatterns: string[] = [`${distDirPosix}/types/**/*.ts`] | ||
|
|
||
| // When isolatedDevBuild is enabled, include both .next/types and .next/dev/types | ||
| // to avoid tsconfig churn when switching between dev/build modes | ||
| if (isolatedDevBuild) { | ||
| typeGlobPatterns.push( | ||
| process.env.NODE_ENV === 'development' | ||
| ? // In dev, distDir is "{distDir}/dev", so also include "{distDir}/types" | ||
| `${distDirPosix.replace(/\/dev$/, '')}/types/**/*.ts` | ||
| : // In build, distDir is "{distDir}", so also include "{distDir}/dev/types" | ||
| `${distDirPosix}/dev/types/**/*.ts` | ||
| ) | ||
| // Sort for consistent order | ||
| typeGlobPatterns.sort((a, b) => a.length - b.length) | ||
| } | ||
|
|
||
| return typeGlobPatterns | ||
| } | ||
|
|
||
| /** | ||
| * Gets the absolute path to the dev types directory for filtering during type-checking. | ||
| * Returns null if isolatedDevBuild is disabled or in dev mode (where dev types are the main types). | ||
| */ | ||
| export function getDevTypesPath( | ||
| baseDir: string, | ||
| distDir: string, | ||
| isolatedDevBuild: boolean | ||
| ): string | null { | ||
| if (!isolatedDevBuild) { | ||
| return null | ||
| } | ||
|
|
||
| const isDev = process.env.NODE_ENV === 'development' | ||
| if (isDev) { | ||
| // In dev mode, dev types are the main types, so no need to filter | ||
| return null | ||
| } | ||
|
|
||
| // In build mode, dev types are at "{baseDir}/{distDir}/dev/types" and should be filtered | ||
| return path.join(baseDir, distDir, 'dev', 'types') | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| export default function RootLayout({ | ||
| children, | ||
| }: { | ||
| children: React.ReactNode | ||
| }) { | ||
| return ( | ||
| <html> | ||
| <body>{children}</body> | ||
| </html> | ||
| ) | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| export default function Page() { | ||
| return <div>Home</div> | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| export default function TempRoutePage() { | ||
| return <div>Temp Route</div> | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| import { nextTestSetup } from 'e2e-utils' | ||
| import { retry } from 'next-test-utils' | ||
|
|
||
| describe('stale-dev-types', () => { | ||
| const { next } = nextTestSetup({ | ||
| files: __dirname, | ||
| }) | ||
|
|
||
| it('should not fail build when .next/dev has stale types from deleted routes', async () => { | ||
| // Step 1: Wait for dev server to generate .next/dev/types/validator.ts | ||
| await retry( | ||
| async () => { | ||
| const exists = await next | ||
| .readFile('.next/dev/types/validator.ts') | ||
| .then(() => true) | ||
| .catch(() => false) | ||
| if (!exists) { | ||
| throw new Error('validator.ts not generated yet') | ||
| } | ||
| }, | ||
| 5000, | ||
| 500 | ||
| ) | ||
|
|
||
| // Verify validator.ts contains reference to temp-route | ||
| const validatorContent = await next.readFile('.next/dev/types/validator.ts') | ||
| expect(validatorContent).toContain('temp-route/page') | ||
|
|
||
| // Step 2: Stop dev server | ||
| await next.stop() | ||
|
|
||
| // Step 3: Delete the temp-route (simulating user deleting a route) | ||
| await next.deleteFile('app/temp-route/page.tsx') | ||
|
|
||
| // Verify .next/dev/types/validator.ts still references deleted route (stale) | ||
| const staleValidator = await next.readFile('.next/dev/types/validator.ts') | ||
| expect(staleValidator).toContain('temp-route/page') | ||
|
|
||
| // Step 4: Run build - should NOT fail due to stale .next/dev types | ||
| const { exitCode, cliOutput } = await next.build() | ||
|
|
||
| // Build should succeed - stale dev types should be excluded from type checking | ||
| expect(cliOutput).not.toContain( | ||
| "Cannot find module '../../../app/temp-route/page" | ||
| ) | ||
| expect(exitCode).toBe(0) | ||
| }) | ||
| }) |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -21,6 +21,105 @@ export class NextDevInstance extends NextInstance { | |||||||||||||||||||||||||
| return this._cliOutput || '' | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| private handleStdio = (childProcess) => { | ||||||||||||||||||||||||||
| childProcess.stdout.on('data', (chunk) => { | ||||||||||||||||||||||||||
| const msg = chunk.toString() | ||||||||||||||||||||||||||
| process.stdout.write(chunk) | ||||||||||||||||||||||||||
| this._cliOutput += msg | ||||||||||||||||||||||||||
| this.emit('stdout', [msg]) | ||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||
| childProcess.stderr.on('data', (chunk) => { | ||||||||||||||||||||||||||
| const msg = chunk.toString() | ||||||||||||||||||||||||||
| process.stderr.write(chunk) | ||||||||||||||||||||||||||
| this._cliOutput += msg | ||||||||||||||||||||||||||
| this.emit('stderr', [msg]) | ||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| private getBuildArgs(args?: string[]) { | ||||||||||||||||||||||||||
| let buildArgs = ['pnpm', 'next', 'build'] | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| if (this.buildCommand) { | ||||||||||||||||||||||||||
| buildArgs = this.buildCommand.split(' ') | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| if (this.buildArgs) { | ||||||||||||||||||||||||||
| buildArgs.push(...this.buildArgs) | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| if (args) { | ||||||||||||||||||||||||||
| buildArgs.push(...args) | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| if (process.env.NEXT_SKIP_ISOLATE) { | ||||||||||||||||||||||||||
| // without isolation yarn can't be used and pnpm must be used instead | ||||||||||||||||||||||||||
| if (buildArgs[0] === 'yarn') { | ||||||||||||||||||||||||||
| buildArgs[0] = 'pnpm' | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| return buildArgs | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| private getSpawnOpts( | ||||||||||||||||||||||||||
| env?: Record<string, string> | ||||||||||||||||||||||||||
| ): import('child_process').SpawnOptions { | ||||||||||||||||||||||||||
| return { | ||||||||||||||||||||||||||
| cwd: this.testDir, | ||||||||||||||||||||||||||
| stdio: ['ignore', 'pipe', 'pipe'], | ||||||||||||||||||||||||||
| shell: false, | ||||||||||||||||||||||||||
| env: { | ||||||||||||||||||||||||||
| ...process.env, | ||||||||||||||||||||||||||
| ...this.env, | ||||||||||||||||||||||||||
| ...env, | ||||||||||||||||||||||||||
| NODE_ENV: this.env.NODE_ENV || ('' as any), | ||||||||||||||||||||||||||
| PORT: this.forcedPort || '0', | ||||||||||||||||||||||||||
| __NEXT_TEST_MODE: 'e2e', | ||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| public async build( | ||||||||||||||||||||||||||
| options: { env?: Record<string, string>; args?: string[] } = {} | ||||||||||||||||||||||||||
| ) { | ||||||||||||||||||||||||||
| if (this.childProcess) { | ||||||||||||||||||||||||||
| throw new Error( | ||||||||||||||||||||||||||
| `can not run build while server is running, use next.stop() first` | ||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| return new Promise<{ | ||||||||||||||||||||||||||
| exitCode: NodeJS.Signals | number | null | ||||||||||||||||||||||||||
| cliOutput: string | ||||||||||||||||||||||||||
| }>((resolve) => { | ||||||||||||||||||||||||||
| const curOutput = this._cliOutput.length | ||||||||||||||||||||||||||
| const spawnOpts = this.getSpawnOpts(options.env) | ||||||||||||||||||||||||||
| const buildArgs = this.getBuildArgs(options.args) | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| console.log('running', shellQuote(buildArgs)) | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| this.childProcess = spawn(buildArgs[0], buildArgs.slice(1), spawnOpts) | ||||||||||||||||||||||||||
| this.handleStdio(this.childProcess) | ||||||||||||||||||||||||||
|
Comment on lines
+101
to
+102
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The spawn process lacks error handling. If Fix by adding error handling: this.childProcess = spawn(buildArgs[0], buildArgs.slice(1), spawnOpts)
this.handleStdio(this.childProcess)
this.childProcess.on('error', (error) => {
this.childProcess = undefined
resolve({
exitCode: 1,
cliOutput: this.cliOutput.slice(curOutput) + '\nSpawn error: ' + error.message
})
})
Suggested change
Spotted by Graphite Agent
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this looks good to add as well |
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| this.childProcess.on('error', (error) => { | ||||||||||||||||||||||||||
| this.childProcess = undefined | ||||||||||||||||||||||||||
| resolve({ | ||||||||||||||||||||||||||
| exitCode: 1, | ||||||||||||||||||||||||||
| cliOutput: | ||||||||||||||||||||||||||
| this.cliOutput.slice(curOutput) + '\nSpawn error: ' + error.message, | ||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| this.childProcess.on('exit', (code, signal) => { | ||||||||||||||||||||||||||
| this.childProcess = undefined | ||||||||||||||||||||||||||
| resolve({ | ||||||||||||||||||||||||||
| exitCode: signal || code, | ||||||||||||||||||||||||||
| cliOutput: this.cliOutput.slice(curOutput), | ||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| public async start() { | ||||||||||||||||||||||||||
| if (this.childProcess) { | ||||||||||||||||||||||||||
| throw new Error('next already started') | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We got the first use case that required doing a production build based on the dev setup, so filling in the unimplemented
buildmethod here.