Skip to content

Commit

Permalink
Fix conflict dev entry key between app and pages (#46832)
Browse files Browse the repository at this point in the history
Add unique identifier `@app@` / `@pages@` / `@root@` for entry key of on
demand entries, so that they'll be unique for each path when the page
key is similar bewteen app and pages like (`"app/page"` and
`"pages/page"` will both end up with `/page`)


## Bug

Follow up for #46736
Closes NEXT-472

- [x] 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)

---------
  • Loading branch information
huozhi authored Mar 7, 2023
1 parent abf7da3 commit 030fd1d
Show file tree
Hide file tree
Showing 4 changed files with 73 additions and 31 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
getInvalidator,
getEntries,
EntryTypes,
getEntryKey,
} from '../../../server/dev/on-demand-entry-handler'
import { WEBPACK_LAYERS } from '../../../lib/constants'
import {
Expand Down Expand Up @@ -629,7 +630,7 @@ export class FlightClientEntryPlugin {
// Inject the entry to the client compiler.
if (this.dev) {
const entries = getEntries(compiler.outputPath)
const pageKey = COMPILER_NAMES.client + bundlePath
const pageKey = getEntryKey(COMPILER_NAMES.client, 'app', bundlePath)

if (!entries[pageKey]) {
entries[pageKey] = {
Expand Down
10 changes: 7 additions & 3 deletions packages/next/src/server/dev/hot-reloader.ts
Original file line number Diff line number Diff line change
Expand Up @@ -659,7 +659,7 @@ export default class HotReloader {
const defaultEntry = config.entry
config.entry = async (...args) => {
const outputPath = this.multiCompiler?.outputPath || ''
const entries: ReturnType<typeof getEntries> = getEntries(outputPath)
const entries = getEntries(outputPath)
// @ts-ignore entry is always a function
const entrypoints = await defaultEntry(...args)
const isClientCompilation = config.name === COMPILER_NAMES.client
Expand All @@ -672,8 +672,12 @@ export default class HotReloader {
const entryData = entries[entryKey]
const { bundlePath, dispose } = entryData

const result = /^(client|server|edge-server)(.*)/g.exec(entryKey)
const [, key, page] = result! // this match should always happen
const result =
/^(client|server|edge-server)@(app|pages|root)@(.*)/g.exec(
entryKey
)
const [, key /* pageType*/, , page] = result! // this match should always happen

if (key === COMPILER_NAMES.client && !isClientCompilation) return
if (key === COMPILER_NAMES.server && !isNodeServerCompilation)
return
Expand Down
81 changes: 61 additions & 20 deletions packages/next/src/server/dev/on-demand-entry-handler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,33 @@ function convertDynamicParamTypeToSyntax(
}
}

/**
* format: {compiler type}@{page type}@{page path}
* e.g. client@pages@/index
* e.g. server@app@app/page
*
* This guarantees the uniqueness for each page, to avoid conflicts between app/ and pages/
*/

export function getEntryKey(
compilerType: CompilerNameValues,
pageBundleType: 'app' | 'pages' | 'root',
page: string
) {
return `${compilerType}@${pageBundleType}@${page}`
}

function getPageBundleType(pageBundlePath: string) {
// Handle special case for /_error
if (pageBundlePath === '/_error') return 'pages'
if (isMiddlewareFilename(pageBundlePath)) return 'root'
return pageBundlePath.startsWith('pages/')
? 'pages'
: pageBundlePath.startsWith('app/')
? 'app'
: 'root'
}

function getEntrypointsFromTree(
tree: FlightRouterState,
isFirst: boolean,
Expand Down Expand Up @@ -479,14 +506,18 @@ export function onDemandEntryHandler({
const pagePaths: string[] = []
for (const entrypoint of entrypoints.values()) {
const page = getRouteFromEntrypoint(entrypoint.name!, root)

if (page) {
pagePaths.push(`${type}${page}`)
const pageBundleType = entrypoint.name?.startsWith('app/')
? 'app'
: 'pages'
pagePaths.push(getEntryKey(type, pageBundleType, page))
} else if (
(root && entrypoint.name === 'root') ||
isMiddlewareFilename(entrypoint.name) ||
isInstrumentationHookFilename(entrypoint.name)
) {
pagePaths.push(`${type}/${entrypoint.name}`)
pagePaths.push(getEntryKey(type, 'root', `/${entrypoint.name}`))
}
}
return pagePaths
Expand Down Expand Up @@ -558,8 +589,8 @@ export function onDemandEntryHandler({
COMPILER_NAMES.server,
COMPILER_NAMES.edgeServer,
]) {
const pageKey = `${compilerType}/${page}`
const entryInfo = curEntries[pageKey]
const entryKey = getEntryKey(compilerType, 'app', `/${page}`)
const entryInfo = curEntries[entryKey]

// If there's no entry, it may have been invalidated and needs to be re-built.
if (!entryInfo) {
Expand All @@ -571,8 +602,8 @@ export function onDemandEntryHandler({
if (entryInfo.status !== BUILT) continue

// If there's an entryInfo
if (!lastServerAccessPagesForAppDir.includes(pageKey)) {
lastServerAccessPagesForAppDir.unshift(pageKey)
if (!lastServerAccessPagesForAppDir.includes(entryKey)) {
lastServerAccessPagesForAppDir.unshift(entryKey)

// Maintain the buffer max length
// TODO: verify that the current pageKey is not at the end of the array as multiple entrypoints can exist
Expand All @@ -598,8 +629,8 @@ export function onDemandEntryHandler({
COMPILER_NAMES.server,
COMPILER_NAMES.edgeServer,
]) {
const pageKey = `${compilerType}${page}`
const entryInfo = curEntries[pageKey]
const entryKey = getEntryKey(compilerType, 'pages', page)
const entryInfo = curEntries[entryKey]

// If there's no entry, it may have been invalidated and needs to be re-built.
if (!entryInfo) {
Expand All @@ -617,8 +648,8 @@ export function onDemandEntryHandler({
if (entryInfo.status !== BUILT) continue

// If there's an entryInfo
if (!lastClientAccessPages.includes(pageKey)) {
lastClientAccessPages.unshift(pageKey)
if (!lastClientAccessPages.includes(entryKey)) {
lastClientAccessPages.unshift(entryKey)

// Maintain the buffer max length
if (lastClientAccessPages.length > pagesBufferLength) {
Expand Down Expand Up @@ -670,14 +701,20 @@ export function onDemandEntryHandler({
const isInsideAppDir =
!!appDir && pagePathData.absolutePagePath.startsWith(appDir)

const pageType = isInsideAppDir ? 'app' : 'pages'
const pageBundleType = getPageBundleType(pagePathData.bundlePath)
const addEntry = (
compilerType: CompilerNameValues
): {
entryKey: string
newEntry: boolean
shouldInvalidate: boolean
} => {
const entryKey = `${compilerType}${pagePathData.page}`
const entryKey = getEntryKey(
compilerType,
pageBundleType,
pagePathData.page
)
if (
curEntries[entryKey] &&
// there can be an overlap in the entryKey for the instrumentation hook file and a page named the same
Expand Down Expand Up @@ -723,21 +760,17 @@ export function onDemandEntryHandler({
pageFilePath: pagePathData.absolutePagePath,
nextConfig,
isDev: true,
pageType: isInsideAppDir ? 'app' : 'pages',
pageType,
})

const added = new Map<CompilerNameValues, ReturnType<typeof addEntry>>()
const isServerComponent =
isInsideAppDir && staticInfo.rsc !== RSC_MODULE_TYPES.client
const pageType = pagePathData.bundlePath.startsWith('pages/')
? 'pages'
: pagePathData.bundlePath.startsWith('app/')
? 'app'
: 'root'

await runDependingOnPageType({
page: pagePathData.page,
pageRuntime: staticInfo.runtime,
pageType,
pageType: pageBundleType,
onClient: () => {
// Skip adding the client entry for app / Server Components.
if (isServerComponent || isInsideAppDir) {
Expand All @@ -747,7 +780,11 @@ export function onDemandEntryHandler({
},
onServer: () => {
added.set(COMPILER_NAMES.server, addEntry(COMPILER_NAMES.server))
const edgeServerEntry = `${COMPILER_NAMES.edgeServer}${pagePathData.page}`
const edgeServerEntry = getEntryKey(
COMPILER_NAMES.edgeServer,
pageBundleType,
pagePathData.page
)
if (
curEntries[edgeServerEntry] &&
!isInstrumentationHookFile(pagePathData.page)
Expand All @@ -761,7 +798,11 @@ export function onDemandEntryHandler({
COMPILER_NAMES.edgeServer,
addEntry(COMPILER_NAMES.edgeServer)
)
const serverEntry = `${COMPILER_NAMES.server}${pagePathData.page}`
const serverEntry = getEntryKey(
COMPILER_NAMES.server,
pageBundleType,
pagePathData.page
)
if (
curEntries[serverEntry] &&
!isInstrumentationHookFile(pagePathData.page)
Expand Down
10 changes: 3 additions & 7 deletions test/e2e/app-dir/similar-pages-paths/similar-pages-paths.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,9 @@ createNextDescribe(
files: __dirname,
skipDeployment: true,
},
({ next, isNextDev }) => {
// TODO: enable development test
if (isNextDev) {
it('should skip dev test', () => {})
return
}
it('should redirect route when requesting it directly', async () => {
({ next }) => {
it('should not have conflicts for similar pattern page paths between app and pages', async () => {
// pages/page and app/page
const res1 = await next.fetch('/')
expect(res1.status).toBe(200)
expect(await res1.text()).toContain('(app/page.js)')
Expand Down

0 comments on commit 030fd1d

Please sign in to comment.