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

Print error when next dev has conflicting app & page #41656

Merged
merged 7 commits into from Oct 24, 2022
Merged
Show file tree
Hide file tree
Changes from 5 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
9 changes: 5 additions & 4 deletions packages/next/build/index.ts
Expand Up @@ -564,7 +564,7 @@ export default async function build(

for (const appPath of pageKeys.app) {
if (pageKeys.pages.includes(appPath)) {
conflictingAppPagePaths.push(`pages${appPath} - app${appPath}`)
conflictingAppPagePaths.push(appPath)
}
}
const numConflicting = conflictingAppPagePaths.length
Expand All @@ -573,10 +573,11 @@ export default async function build(
Log.error(
`Conflicting app and page file${
numConflicting === 1 ? ' was' : 's were'
} found, please remove the conflicting files to continue. \n${conflictingAppPagePaths.join(
'\n'
)}\n`
} found, please remove the conflicting files to continue:`
)
for (const p of conflictingAppPagePaths) {
Log.error(` "pages${p}" - "app${p}"`)
}
process.exit(1)
}
}
Expand Down
41 changes: 34 additions & 7 deletions packages/next/server/dev/next-dev-server.ts
Expand Up @@ -322,6 +322,8 @@ export default class DevServer extends Server {
const knownFiles = wp.getTimeInfoEntries()
const appPaths: Record<string, string[]> = {}
const edgeRoutesSet = new Set<string>()
const pageNameSet = new Set<string>()
const conflictingAppPagePaths: string[] = []

let envChange = false
let tsconfigChange = false
Expand Down Expand Up @@ -419,6 +421,12 @@ export default class DevServer extends Server {
pageName = pageName.replace(/\/index$/, '') || '/'
}

if (pageNameSet.has(pageName)) {
conflictingAppPagePaths.push(pageName)
} else {
pageNameSet.add(pageName)
}

/**
* If there is a middleware that is not declared in the root we will
* warn without adding it so it doesn't make its way into the system.
Expand All @@ -442,6 +450,19 @@ export default class DevServer extends Server {
})
}

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)
}

if (!this.usingTypeScript && enabledTypeScript) {
// we tolerate the error here as this is best effort
// and the manual install command will be shown
Expand Down Expand Up @@ -751,27 +772,33 @@ export default class DevServer extends Server {
).then(Boolean)
}

// check appDir first if enabled
let appFile: string | null = null
let pagesFile: string | null = null

if (this.appDir) {
const pageFile = await findPageFile(
appFile = await findPageFile(
this.appDir,
normalizedPath,
normalizedPath + '/page',
this.nextConfig.pageExtensions,
true
)
if (pageFile) return true
}

if (this.pagesDir) {
const pageFile = await findPageFile(
pagesFile = await findPageFile(
this.pagesDir,
normalizedPath,
this.nextConfig.pageExtensions,
false
)
return !!pageFile
}
return false
if (appFile && pagesFile) {
throw new Error(
`Conflicting app and page file found: "app${appFile}" and "pages${pagesFile}". Please remove one to continue.`
)
}

return Boolean(appFile || pagesFile)
}

protected async _beforeCatchAllRender(
Expand Down
10 changes: 10 additions & 0 deletions test/integration/conflicting-app-page-error/app/layout.js
@@ -0,0 +1,10 @@
export default function RootLayout({ children }) {
return (
<html>
<head>
<title>Conflict Test</title>
</head>
<body>{children}</body>
</html>
)
}
@@ -1,3 +1,3 @@
export default function Page(props) {
return <p>another app</p>
return <p>non-conflict app</p>
}
3 changes: 3 additions & 0 deletions test/integration/conflicting-app-page-error/pages/index.js
@@ -0,0 +1,3 @@
export default function Page(props) {
return <p>index page</p>
}
98 changes: 87 additions & 11 deletions test/integration/conflicting-app-page-error/test/index.test.js
@@ -1,24 +1,100 @@
/* eslint-env jest */

import path from 'path'
import { nextBuild } from 'next-test-utils'
import {
killApp,
findPort,
getRedboxHeader,
hasRedbox,
launchApp,
nextBuild,
waitFor,
} from 'next-test-utils'
import webdriver from 'next-webdriver'

let app
let appPort
const appDir = path.join(__dirname, '..')
let output = ''

describe('conflict between app file and page file', () => {
it('errors during build', async () => {
const conflicts = ['/hello', '/another']
const results = await nextBuild(appDir, [], {
stdout: true,
stderr: true,
env: { NEXT_SKIP_APP_REACT_INSTALL: '1' },
})
const output = results.stdout + results.stderr
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 conflicts) {
for (const conflict of ['/hello', '/another']) {
expect(output).toContain(conflict)
}
expect(output).not.toContain('/index')
expect(output).not.toContain('/non-conflict')
})

if (dev) {
it('should show error overlay for /hello', async () => {
const browser = await webdriver(appPort, '/hello')
expect(await hasRedbox(browser, true)).toBe(true)
expect(await getRedboxHeader(browser)).toContain(
'Conflicting app and page file found: "app/hello/page.js" and "pages/hello.js". Please remove one to continue.'
)
})

it('should show error overlay for /another', async () => {
const browser = await webdriver(appPort, '/another')
expect(await hasRedbox(browser, true)).toBe(true)
expect(await getRedboxHeader(browser)).toContain(
'Conflicting app and page file found: "app/another/page.js" and "pages/another.js". Please remove one to continue.'
)
})

it('should not show error overlay for /', async () => {
const browser = await webdriver(appPort, '/')
expect(await hasRedbox(browser, false)).toBe(false)
expect(await getRedboxHeader(browser)).toBe(null)
expect(await browser.elementByCss('p').text()).toBe('index page')
})

it('should not show error overlay for /non-conflict', async () => {
const browser = await webdriver(appPort, '/non-conflict')
expect(await hasRedbox(browser, false)).toBe(false)
expect(await getRedboxHeader(browser)).toBe(null)
expect(await browser.elementByCss('p').text()).toBe('non-conflict app')
})
}
}

describe('Conflict between app file and page file', () => {
describe('next dev', () => {
beforeAll(async () => {
output = ''
appPort = await findPort()
app = await launchApp(appDir, appPort, {
onStdout(msg) {
output += msg || ''
},
onStderr(msg) {
output += msg || ''
},
})
await waitFor(800)
})
afterAll(() => {
killApp(app)
})
runTests({ dev: true })
})

describe('next build', () => {
beforeAll(async () => {
output = ''
const { stdout, stderr } = await nextBuild(appDir, [], {
stdout: true,
stderr: true,
env: { NEXT_SKIP_APP_REACT_INSTALL: '1' },
})
output = stdout + stderr
})
afterAll(() => {
killApp(app)
})
runTests({ dev: false })
})
})