Skip to content

Commit

Permalink
App files ending with page registred as page files (#42996)
Browse files Browse the repository at this point in the history
When building (`next/build/index.ts`) files ending with `page.js` got
picked up by the RegEx which meant files like `not-a-page.js` ended up
as a page. This change makes sure the segment actually starts with
`page`.

Dev had the same issue and also missed taking `pageExtensions` into
account.

Fixes #42972

## 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)
  • Loading branch information
Hannes Bornö committed Nov 16, 2022
1 parent ec07e30 commit fdc07c0
Show file tree
Hide file tree
Showing 7 changed files with 26 additions and 14 deletions.
2 changes: 1 addition & 1 deletion packages/next/build/index.ts
Expand Up @@ -493,7 +493,7 @@ export default async function build(
.traceAsyncFn(() =>
recursiveReadDir(
appDir,
new RegExp(`page\\.(?:${config.pageExtensions.join('|')})$`)
new RegExp(`^page\\.(?:${config.pageExtensions.join('|')})$`)
)
)
}
Expand Down
2 changes: 1 addition & 1 deletion packages/next/server/dev/next-dev-server.ts
Expand Up @@ -405,7 +405,7 @@ export default class DevServer extends Server {
})

if (isAppPath) {
if (!isLayoutsLeafPage(fileName)) {
if (!isLayoutsLeafPage(fileName, this.nextConfig.pageExtensions)) {
continue
}

Expand Down
10 changes: 5 additions & 5 deletions packages/next/server/lib/find-page-file.ts
Expand Up @@ -64,9 +64,9 @@ export async function findPageFile(
}

// Determine if the file is leaf node page file under layouts,
// The filename should start with 'page', it can be either shared,
// client, or server components with allowed page file extension.
// e.g. page.js, page.server.js, page.client.tsx, etc.
export function isLayoutsLeafPage(filePath: string) {
return /[\\/]?page\.((server|client)\.?)?[jt]sx?$/.test(filePath)
// The filename should start with 'page' and end with one of the allowed extensions
export function isLayoutsLeafPage(filePath: string, pageExtensions: string[]) {
return new RegExp(`(^page|/page)\\.(?:${pageExtensions.join('|')})$`).test(
filePath
)
}
3 changes: 3 additions & 0 deletions test/e2e/app-dir/app/app/catch-all/[...slug]/not-a-page.js
@@ -0,0 +1,3 @@
export default function NotAPage() {
return <h1 id="not-a-page">Not a page</h1>
}
2 changes: 2 additions & 0 deletions test/e2e/app-dir/app/app/catch-all/[...slug]/page.js
@@ -1,4 +1,5 @@
import Widget from './components/widget'
import NotAPage from './not-a-page'

export default function Page({ params }) {
return (
Expand All @@ -7,6 +8,7 @@ export default function Page({ params }) {
hello from /catch-all/{params.slug.join('/')}
</h1>
<Widget />
<NotAPage />
</>
)
}
1 change: 1 addition & 0 deletions test/e2e/app-dir/index.test.ts
Expand Up @@ -726,6 +726,7 @@ describe('app dir', () => {
const html = await renderViaHTTP(next.url, `/catch-all/${route}`)
const $ = cheerio.load(html)
expect($('#text').attr('data-params')).toBe(route)
expect($('#not-a-page').text()).toBe('Not a page')

// Components under catch-all should not be treated as route that errors during build.
// They should be rendered properly when imported in page route.
Expand Down
20 changes: 13 additions & 7 deletions test/unit/find-page-file.test.ts
Expand Up @@ -46,16 +46,22 @@ describe('findPageFile', () => {
})

describe('isLayoutsLeafPage', () => {
const pageExtensions = ['tsx', 'ts', 'jsx', 'js']
it('should determine either server or client component page file as leaf node page', () => {
expect(isLayoutsLeafPage('page.js')).toBe(true)
expect(isLayoutsLeafPage('./page.server.js')).toBe(true)
expect(isLayoutsLeafPage('./page.server.jsx')).toBe(true)
expect(isLayoutsLeafPage('./page.client.ts')).toBe(true)
expect(isLayoutsLeafPage('./page.client.tsx')).toBe(true)
expect(isLayoutsLeafPage('page.js', pageExtensions)).toBe(true)
expect(isLayoutsLeafPage('./page.js', pageExtensions)).toBe(true)
expect(isLayoutsLeafPage('./page.jsx', pageExtensions)).toBe(true)
expect(isLayoutsLeafPage('/page.ts', pageExtensions)).toBe(true)
expect(isLayoutsLeafPage('/path/page.tsx', pageExtensions)).toBe(true)
})

it('should determine other files under layout routes as non leaf node', () => {
expect(isLayoutsLeafPage('./page.component.jsx')).toBe(false)
expect(isLayoutsLeafPage('layout.js')).toBe(false)
expect(isLayoutsLeafPage('./not-a-page.js', pageExtensions)).toBe(false)
expect(isLayoutsLeafPage('not-a-page.js', pageExtensions)).toBe(false)
expect(isLayoutsLeafPage('./page.component.jsx', pageExtensions)).toBe(
false
)
expect(isLayoutsLeafPage('layout.js', pageExtensions)).toBe(false)
expect(isLayoutsLeafPage('page', pageExtensions)).toBe(false)
})
})

0 comments on commit fdc07c0

Please sign in to comment.