From e74de1a46c0353c5ba8266b6aed90bcf6f8da059 Mon Sep 17 00:00:00 2001 From: Tim Neutkens Date: Thu, 3 Nov 2022 20:50:39 +0100 Subject: [PATCH] Clarify app and pages file conflicting files (#42415) Improvement to how the `app` and `pages` files conflict is shown. Especially the last log line `"pages/" - "app/"` made it seem like you should remove the `pages` folder altogether. This was a bug in how the `''` case was displayed. After having a look at this I went further and added exactly which file caused the conflict given that `app` allows you to create `app/(home)/page.js` and such it saves some digging for what the actual conflicting file is. Similarly in `pages` both `pages/dashboard/index.js` and `pages/dashboard.js` are possible. Before: ``` error - Conflicting app and page files were found, please remove the conflicting files to continue: error - "pages/another" - "app/another" error - "pages/hello" - "app/hello" error - "pages/" - "app/" ``` After: ``` error - Conflicting app and page files were found, please remove the conflicting files to continue: error - "pages/another.js" - "app/another/page.js" error - "pages/index.js" - "app/page.js" error - "pages/hello.js" - "app/hello/page.js" ``` ## Bug - [ ] Related issues linked using `fixes #number` - [ ] Integration tests added - [ ] Errors have a helpful link attached, see `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` - [ ] Integration 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` ## 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) Co-authored-by: JJ Kasper --- packages/next/build/index.ts | 58 ++++++++++--------- packages/next/server/dev/next-dev-server.ts | 20 +++++-- .../conflicting-app-page-error/app/page.js | 3 + .../pages/non-conflict-pages.js | 3 + .../test/index.test.js | 24 ++++++-- 5 files changed, 70 insertions(+), 38 deletions(-) create mode 100644 test/integration/conflicting-app-page-error/app/page.js create mode 100644 test/integration/conflicting-app-page-error/pages/non-conflict-pages.js diff --git a/packages/next/build/index.ts b/packages/next/build/index.ts index c9d6241b8facc7d..46802a753c1ae22 100644 --- a/packages/next/build/index.ts +++ b/packages/next/build/index.ts @@ -576,36 +576,42 @@ export default async function build( }) ) - const pageKeys = { - pages: Object.keys(mappedPages), - app: mappedAppPages - ? Object.keys(mappedAppPages).map( - (key) => normalizeAppPath(key) || '/' - ) - : undefined, - } - - if (pageKeys.app) { - const conflictingAppPagePaths = [] - - for (const appPath of pageKeys.app) { - if (pageKeys.pages.includes(appPath)) { - conflictingAppPagePaths.push(appPath) + const pagesPageKeys = Object.keys(mappedPages) + + const conflictingAppPagePaths: [pagePath: string, appPath: string][] = [] + const appPageKeys: string[] = [] + if (mappedAppPages) { + for (const appKey in mappedAppPages) { + const normalizedAppPageKey = normalizeAppPath(appKey) || '/' + const pagePath = mappedPages[normalizedAppPageKey] + if (pagePath) { + const appPath = mappedAppPages[appKey] + conflictingAppPagePaths.push([ + pagePath.replace(/^private-next-pages/, 'pages'), + appPath.replace(/^private-next-app-dir/, 'app'), + ]) } + + appPageKeys.push(normalizedAppPageKey) } - const numConflicting = conflictingAppPagePaths.length + } - if (numConflicting > 0) { - Log.error( - `Conflicting app and page file${ - numConflicting === 1 ? ' was' : 's were' - } found, please remove the conflicting files to continue:` - ) - for (const p of conflictingAppPagePaths) { - Log.error(` "pages${p}" - "app${p}"`) - } - process.exit(1) + const pageKeys = { + pages: pagesPageKeys, + app: appPageKeys.length > 0 ? appPageKeys : undefined, + } + + const numConflictingAppPaths = conflictingAppPagePaths.length + if (mappedAppPages && numConflictingAppPaths > 0) { + Log.error( + `Conflicting app and page file${ + numConflictingAppPaths === 1 ? ' was' : 's were' + } found, please remove the conflicting files to continue:` + ) + for (const [pagePath, appPath] of conflictingAppPagePaths) { + Log.error(` "${pagePath}" - "${appPath}"`) } + process.exit(1) } const conflictingPublicFiles: string[] = [] diff --git a/packages/next/server/dev/next-dev-server.ts b/packages/next/server/dev/next-dev-server.ts index 8fa0e5c9712f2d1..0a1f86351132e59 100644 --- a/packages/next/server/dev/next-dev-server.ts +++ b/packages/next/server/dev/next-dev-server.ts @@ -323,7 +323,9 @@ export default class DevServer extends Server { const appPaths: Record = {} const edgeRoutesSet = new Set() const pageNameSet = new Set() - const conflictingAppPagePaths: string[] = [] + const conflictingAppPagePaths = new Set() + const appPageFilePaths = new Map() + const pagesPageFilePaths = new Map() let envChange = false let tsconfigChange = false @@ -422,8 +424,13 @@ export default class DevServer extends Server { pageName = pageName.replace(/\/index$/, '') || '/' } - if (pageNameSet.has(pageName)) { - conflictingAppPagePaths.push(pageName) + ;(isAppPath ? appPageFilePaths : pagesPageFilePaths).set( + pageName, + fileName + ) + + if (this.appDir && pageNameSet.has(pageName)) { + conflictingAppPagePaths.add(pageName) } else { pageNameSet.add(pageName) } @@ -451,7 +458,7 @@ export default class DevServer extends Server { }) } - const numConflicting = conflictingAppPagePaths.length + const numConflicting = conflictingAppPagePaths.size if (numConflicting > 0) { Log.error( `Conflicting app and page file${ @@ -459,9 +466,10 @@ export default class DevServer extends Server { } found, please remove the conflicting files to continue:` ) for (const p of conflictingAppPagePaths) { - Log.error(` "pages${p}" - "app${p}"`) + const appPath = relative(this.dir, appPageFilePaths.get(p)!) + const pagesPath = relative(this.dir, pagesPageFilePaths.get(p)!) + Log.error(` "${pagesPath}" - "${appPath}"`) } - //process.exit(1) } if (!this.usingTypeScript && enabledTypeScript) { diff --git a/test/integration/conflicting-app-page-error/app/page.js b/test/integration/conflicting-app-page-error/app/page.js new file mode 100644 index 000000000000000..821fdd14d09725f --- /dev/null +++ b/test/integration/conflicting-app-page-error/app/page.js @@ -0,0 +1,3 @@ +export default function Page(props) { + return

index page

+} diff --git a/test/integration/conflicting-app-page-error/pages/non-conflict-pages.js b/test/integration/conflicting-app-page-error/pages/non-conflict-pages.js new file mode 100644 index 000000000000000..86f3a93651b382c --- /dev/null +++ b/test/integration/conflicting-app-page-error/pages/non-conflict-pages.js @@ -0,0 +1,3 @@ +export default function Page() { + return

Hello World!

+} diff --git a/test/integration/conflicting-app-page-error/test/index.test.js b/test/integration/conflicting-app-page-error/test/index.test.js index 3a12a3a397d6b27..c2b74218e85d591 100644 --- a/test/integration/conflicting-app-page-error/test/index.test.js +++ b/test/integration/conflicting-app-page-error/test/index.test.js @@ -21,14 +21,26 @@ function runTests({ dev }) { it('should print error for conflicting app/page', async () => { expect(output).toMatch(/Conflicting app and page files were found/) - for (const conflict of ['/hello', '/another']) { - expect(output).toContain(conflict) + for (const [pagePath, appPath] of [ + ['pages/index.js', 'app/page.js'], + ['pages/hello.js', 'app/hello/page.js'], + ['pages/another.js', 'app/another/page.js'], + ]) { + expect(output).toContain(`"${pagePath}" - "${appPath}"`) } - expect(output).not.toContain('/index') + expect(output).not.toContain('/non-conflict-pages') expect(output).not.toContain('/non-conflict') }) if (dev) { + it('should show error overlay for /', async () => { + const browser = await webdriver(appPort, '/') + expect(await hasRedbox(browser, true)).toBe(true) + expect(await getRedboxHeader(browser)).toContain( + 'Conflicting app and page file found: "app/page.js" and "pages/index.js". Please remove one to continue.' + ) + }) + it('should show error overlay for /hello', async () => { const browser = await webdriver(appPort, '/hello') expect(await hasRedbox(browser, true)).toBe(true) @@ -45,11 +57,11 @@ function runTests({ dev }) { ) }) - it('should not show error overlay for /', async () => { - const browser = await webdriver(appPort, '/') + it('should not show error overlay for /non-conflict-pages', async () => { + const browser = await webdriver(appPort, '/non-conflict-pages') expect(await hasRedbox(browser, false)).toBe(false) expect(await getRedboxHeader(browser)).toBe(null) - expect(await browser.elementByCss('p').text()).toBe('index page') + expect(await browser.elementByCss('h1').text()).toBe('Hello World!') }) it('should not show error overlay for /non-conflict', async () => {