From 8b2492b77ec3153edd15ccba0062eb2768bd44b9 Mon Sep 17 00:00:00 2001 From: Shu Ding Date: Wed, 1 Mar 2023 00:32:03 +0100 Subject: [PATCH] Fix app client child entry not being disposed when deleting the file (#46583) Currently if a file or folder (that contains an entry) is renamed in app dir, the dev server will stop working because we never remove the old entry. Since all client entries in app dir are created as child entries programmatically via the RSC plugin, they're different and not handled by our existing hot reloader logic: https://github.com/vercel/next.js/blob/f0cbe84e4c2ed159df70a926d2f8f23b4c1025c9/packages/next/src/server/dev/hot-reloader.ts#L666-L677 This PR adds a file path to child entries as well (it can be layout, page and other entries) so in the entry generation step we can prune the invalid ones. Fixes #46379, fixes NEXT-650. ## Bug - [ ] Related issues linked using `fixes #number` - [x] Integration tests added - [ ] Errors have a helpful link attached, see [`contributing.md`](https://github.com/vercel/next.js/blob/canary/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` - [ ] [e2e](https://github.com/vercel/next.js/blob/canary/contributing/core/testing.md#writing-tests-for-nextjs) 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`](https://github.com/vercel/next.js/blob/canary/contributing.md) ## Documentation / Examples - [ ] Make sure the linting passes by running `pnpm build && pnpm lint` - [ ] The "examples guidelines" are followed from [our contributing doc](https://github.com/vercel/next.js/blob/canary/contributing/examples/adding-examples.md) --- .../plugins/flight-client-entry-plugin.ts | 8 +++- packages/next/src/server/dev/hot-reloader.ts | 13 +++++++ .../next/src/server/dev/next-dev-server.ts | 4 +- .../src/server/dev/on-demand-entry-handler.ts | 13 +++++-- test/development/app-hmr/app/folder/page.jsx | 3 ++ test/development/app-hmr/app/layout.jsx | 7 ++++ test/development/app-hmr/hmr.test.ts | 38 +++++++++++++++++++ test/development/app-hmr/next.config.js | 8 ++++ test/lib/next-modes/base.ts | 6 +++ 9 files changed, 93 insertions(+), 7 deletions(-) create mode 100644 test/development/app-hmr/app/folder/page.jsx create mode 100644 test/development/app-hmr/app/layout.jsx create mode 100644 test/development/app-hmr/hmr.test.ts create mode 100644 test/development/app-hmr/next.config.js diff --git a/packages/next/src/build/webpack/plugins/flight-client-entry-plugin.ts b/packages/next/src/build/webpack/plugins/flight-client-entry-plugin.ts index 9a194f3a4a321..e3a82896164f1 100644 --- a/packages/next/src/build/webpack/plugins/flight-client-entry-plugin.ts +++ b/packages/next/src/build/webpack/plugins/flight-client-entry-plugin.ts @@ -247,7 +247,7 @@ export class FlightClientEntryPlugin { // Replace file suffix as `.js` will be added. const bundlePath = normalizePathSep( - relativeRequest.replace(/\.(js|ts)x?$/, '').replace(/^src[\\/]/, '') + relativeRequest.replace(/\.[^.\\/]+$/, '').replace(/^src[\\/]/, '') ) addClientEntryAndSSRModulesList.push( @@ -257,6 +257,7 @@ export class FlightClientEntryPlugin { entryName: name, clientImports, bundlePath, + absolutePagePath: entryRequest, }) ) } @@ -574,12 +575,14 @@ export class FlightClientEntryPlugin { entryName, clientImports, bundlePath, + absolutePagePath, }: { compiler: webpack.Compiler compilation: webpack.Compilation entryName: string clientImports: ClientComponentImports bundlePath: string + absolutePagePath?: string }): [shouldInvalidate: boolean, addEntryPromise: Promise] { let shouldInvalidate = false @@ -618,6 +621,7 @@ export class FlightClientEntryPlugin { entries[pageKey] = { type: EntryTypes.CHILD_ENTRY, parentEntries: new Set([entryName]), + absoluteEntryFilePath: absolutePagePath, bundlePath, request: clientLoader, dispose: false, @@ -634,6 +638,8 @@ export class FlightClientEntryPlugin { if (entryData.type === EntryTypes.CHILD_ENTRY) { entryData.parentEntries.add(entryName) } + entryData.dispose = false + entryData.lastActiveTime = Date.now() } } else { injectedClientEntries.set(bundlePath, clientLoader) diff --git a/packages/next/src/server/dev/hot-reloader.ts b/packages/next/src/server/dev/hot-reloader.ts index 192cb8736290e..2a72de6f79a65 100644 --- a/packages/next/src/server/dev/hot-reloader.ts +++ b/packages/next/src/server/dev/hot-reloader.ts @@ -676,6 +676,19 @@ export default class HotReloader { } } + // For child entries, if it has an entry file and it's gone, remove it + if (isChildEntry) { + if (entryData.absoluteEntryFilePath) { + const pageExists = + !dispose && + (await fileExists(entryData.absoluteEntryFilePath)) + if (!pageExists) { + delete entries[entryKey] + return + } + } + } + const hasAppDir = !!this.appDir const isAppPath = hasAppDir && bundlePath.startsWith('app/') const staticInfo = isEntry diff --git a/packages/next/src/server/dev/next-dev-server.ts b/packages/next/src/server/dev/next-dev-server.ts index 926b309a668b3..0e410e6ac014e 100644 --- a/packages/next/src/server/dev/next-dev-server.ts +++ b/packages/next/src/server/dev/next-dev-server.ts @@ -1641,8 +1641,8 @@ export default class DevServer extends Server { } async getCompilationError(page: string): Promise { - const errors = (await this.hotReloader?.getCompilationErrors(page)) || [] - if (errors.length === 0) return + const errors = await this.hotReloader?.getCompilationErrors(page) + if (!errors) return // Return the very first error we found. return errors[0] diff --git a/packages/next/src/server/dev/on-demand-entry-handler.ts b/packages/next/src/server/dev/on-demand-entry-handler.ts index 8978ed5f6f314..72b95fb2a6091 100644 --- a/packages/next/src/server/dev/on-demand-entry-handler.ts +++ b/packages/next/src/server/dev/on-demand-entry-handler.ts @@ -167,6 +167,11 @@ interface ChildEntry extends EntryType { * Which parent entries use this childEntry. */ parentEntries: Set + /** + * The absolute page to the entry file. Used for detecting if the file was removed. For example: + * `/Users/Rick/project/app/about/layout.js` + */ + absoluteEntryFilePath?: string } const entriesMap: Map< @@ -498,7 +503,7 @@ export function onDemandEntryHandler({ multiCompiler.hooks.done.tap('NextJsOnDemandEntries', (multiStats) => { const [clientStats, serverStats, edgeServerStats] = multiStats.stats const root = !!appDir - const pagePaths = [ + const entryNames = [ ...getPagePathsFromEntrypoints( COMPILER_NAMES.client, clientStats.compilation.entrypoints, @@ -518,8 +523,8 @@ export function onDemandEntryHandler({ : []), ] - for (const page of pagePaths) { - const entry = curEntries[page] + for (const name of entryNames) { + const entry = curEntries[name] if (!entry) { continue } @@ -529,7 +534,7 @@ export function onDemandEntryHandler({ } entry.status = BUILT - doneCallbacks!.emit(page) + doneCallbacks!.emit(name) } getInvalidator(multiCompiler.outputPath)?.doneBuilding([...COMPILER_KEYS]) diff --git a/test/development/app-hmr/app/folder/page.jsx b/test/development/app-hmr/app/folder/page.jsx new file mode 100644 index 0000000000000..f54faf15c6bcd --- /dev/null +++ b/test/development/app-hmr/app/folder/page.jsx @@ -0,0 +1,3 @@ +export default function Page() { + return

Hello

+} diff --git a/test/development/app-hmr/app/layout.jsx b/test/development/app-hmr/app/layout.jsx new file mode 100644 index 0000000000000..a3a86a5ca1e12 --- /dev/null +++ b/test/development/app-hmr/app/layout.jsx @@ -0,0 +1,7 @@ +export default function Root({ children }) { + return ( + + {children} + + ) +} diff --git a/test/development/app-hmr/hmr.test.ts b/test/development/app-hmr/hmr.test.ts new file mode 100644 index 0000000000000..d96b28e207b7f --- /dev/null +++ b/test/development/app-hmr/hmr.test.ts @@ -0,0 +1,38 @@ +import { createNextDescribe } from 'e2e-utils' +import { check } from 'next-test-utils' + +createNextDescribe( + `app-dir-hmr`, + { + files: __dirname, + }, + ({ next }) => { + describe('filesystem changes', () => { + it('should not break when renaming a folder', async () => { + console.log(next.url) + const browser = await next.browser('/folder') + const text = await browser.elementByCss('h1').text() + expect(text).toBe('Hello') + + // Rename folder + await next.renameFolder('app/folder', 'app/folder-renamed') + + try { + // Should be 404 in a few seconds + await check(async () => { + const body = await browser.elementByCss('body').text() + expect(body).toContain('404') + return 'success' + }, 'success') + + // The new page should be rendered + const newHTML = await next.render('/folder-renamed') + expect(newHTML).toContain('Hello') + } finally { + // Rename it back + await next.renameFolder('app/folder-renamed', 'app/folder') + } + }) + }) + } +) diff --git a/test/development/app-hmr/next.config.js b/test/development/app-hmr/next.config.js new file mode 100644 index 0000000000000..d45ffa44885de --- /dev/null +++ b/test/development/app-hmr/next.config.js @@ -0,0 +1,8 @@ +/** + * @type {import('next').NextConfig} + */ +module.exports = { + experimental: { + appDir: true, + }, +} diff --git a/test/lib/next-modes/base.ts b/test/lib/next-modes/base.ts index dba33065cb65e..e47b77e32ed0a 100644 --- a/test/lib/next-modes/base.ts +++ b/test/lib/next-modes/base.ts @@ -401,6 +401,12 @@ export class NextInstance { path.join(this.testDir, newFilename) ) } + public async renameFolder(foldername: string, newFoldername: string) { + return fs.move( + path.join(this.testDir, foldername), + path.join(this.testDir, newFoldername) + ) + } public async deleteFile(filename: string) { return fs.remove(path.join(this.testDir, filename)) }