Clarify app and pages file conflicting files (#42415)

timneutkens · ijjk · web-flow · commit e74de1a46c03 · 2022-11-03T12:50:39.000-07:00
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 <jj@jjsweb.site>
diff --git 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
@@ -323,7 +323,9 @@ export default class DevServer extends Server {
         const appPaths: Record<string, string[]> = {}
         const edgeRoutesSet = new Set<string>()
         const pageNameSet = new Set<string>()
-        const conflictingAppPagePaths: string[] = []
+        const conflictingAppPagePaths = new Set<string>()
+        const appPageFilePaths = new Map<string, string>()
+        const pagesPageFilePaths = new Map<string, string>()
 
         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,17 +458,18 @@ export default class DevServer extends Server {
           })
         }
 
-        const numConflicting = conflictingAppPagePaths.length
+        const numConflicting = conflictingAppPagePaths.size
         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}"`)
+            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
@@ -0,0 +1,3 @@
+export default function Page(props) {
+  return <p>index page</p>
+}
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
@@ -0,0 +1,3 @@
+export default function Page() {
+  return <h1>Hello World!</h1>
+}
diff --git 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 () => {

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+export default function Page(props) {`
	`2`	`+ return <p>index page</p>`
	`3`	`+}`
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+export default function Page() {`
	`2`	`+ return <h1>Hello World!</h1>`
	`3`	`+}`