Skip to content
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

Fix app client child entry not being disposed when deleting the file #46583

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -257,6 +257,7 @@ export class FlightClientEntryPlugin {
entryName: name,
clientImports,
bundlePath,
absolutePagePath: entryRequest,
})
)
}
Expand Down Expand Up @@ -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<void>] {
let shouldInvalidate = false

Expand Down Expand Up @@ -618,6 +621,7 @@ export class FlightClientEntryPlugin {
entries[pageKey] = {
type: EntryTypes.CHILD_ENTRY,
parentEntries: new Set([entryName]),
absoluteEntryFilePath: absolutePagePath,
bundlePath,
request: clientLoader,
dispose: false,
Expand All @@ -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)
Expand Down
13 changes: 13 additions & 0 deletions packages/next/src/server/dev/hot-reloader.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions packages/next/src/server/dev/next-dev-server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1641,8 +1641,8 @@ export default class DevServer extends Server {
}

async getCompilationError(page: string): Promise<any> {
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]
Expand Down
13 changes: 9 additions & 4 deletions packages/next/src/server/dev/on-demand-entry-handler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,11 @@ interface ChildEntry extends EntryType {
* Which parent entries use this childEntry.
*/
parentEntries: Set<string>
/**
* 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<
Expand Down Expand Up @@ -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,
Expand All @@ -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
}
Expand All @@ -529,7 +534,7 @@ export function onDemandEntryHandler({
}

entry.status = BUILT
doneCallbacks!.emit(page)
doneCallbacks!.emit(name)
}

getInvalidator(multiCompiler.outputPath)?.doneBuilding([...COMPILER_KEYS])
Expand Down
3 changes: 3 additions & 0 deletions test/development/app-hmr/app/folder/page.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default function Page() {
return <h1>Hello</h1>
}
7 changes: 7 additions & 0 deletions test/development/app-hmr/app/layout.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
export default function Root({ children }) {
return (
<html>
<body>{children}</body>
</html>
)
}
38 changes: 38 additions & 0 deletions test/development/app-hmr/hmr.test.ts
Original file line number Diff line number Diff line change
@@ -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')
}
})
})
}
)
8 changes: 8 additions & 0 deletions test/development/app-hmr/next.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
/**
* @type {import('next').NextConfig}
*/
module.exports = {
experimental: {
appDir: true,
},
}
6 changes: 6 additions & 0 deletions test/lib/next-modes/base.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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))
}
Expand Down