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

Treat non page file as non route under app dir #39976

Merged
merged 5 commits into from Aug 26, 2022
Merged
Show file tree
Hide file tree
Changes from 4 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
6 changes: 5 additions & 1 deletion packages/next/server/dev/next-dev-server.ts
Expand Up @@ -44,7 +44,7 @@ import { eventCliSession } from '../../telemetry/events'
import { Telemetry } from '../../telemetry/storage'
import { setGlobal } from '../../trace'
import HotReloader from './hot-reloader'
import { findPageFile } from '../lib/find-page-file'
import { findPageFile, isLayoutsLeafPage } from '../lib/find-page-file'
import { getNodeOptionsWithoutInspect } from '../lib/utils'
import {
UnwrapPromise,
Expand Down Expand Up @@ -387,6 +387,10 @@ export default class DevServer extends Server {
})

if (isAppPath) {
if (!isLayoutsLeafPage(fileName)) {
continue
}

const originalPageName = pageName
pageName = normalizeAppPath(pageName) || '/'
appPaths[pageName] = originalPageName
Expand Down
8 changes: 8 additions & 0 deletions packages/next/server/lib/find-page-file.ts
Expand Up @@ -60,3 +60,11 @@ export async function findPageFile(

return existingPath
}

// Determine if the file is leaf node page file under layouts,
// They 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.
ijjk marked this conversation as resolved.
Show resolved Hide resolved
export function isLayoutsLeafPage(filePath: string) {
return /[\\/]?page\.((server|client)\.?)?[jt]sx?$/.test(filePath)
}
@@ -0,0 +1,2 @@
// components under catch-all should not throw errors
export default () => <p id="widget">widget</p>
11 changes: 8 additions & 3 deletions test/e2e/app-dir/app/app/catch-all/[...slug]/page.server.js
@@ -1,11 +1,16 @@
import Widget from './components/widget'

export function getServerSideProps({ params }) {
return { props: { params } }
}

export default function Page({ params }) {
return (
<h1 id="text" data-params={params.slug.join('/') ?? ''}>
hello from /catch-all/{params.slug.join('/')}
</h1>
<>
<h1 id="text" data-params={params.slug.join('/') ?? ''}>
hello from /catch-all/{params.slug.join('/')}
</h1>
<Widget />
</>
)
}
4 changes: 4 additions & 0 deletions test/e2e/app-dir/index.test.ts
Expand Up @@ -582,6 +582,10 @@ describe('app dir', () => {
const html = await renderViaHTTP(next.url, `/catch-all/${route}`)
const $ = cheerio.load(html)
expect($('#text').attr('data-params')).toBe(route)

// Components under catch-all should not be treated as route that errors during build.
// They should be rendered properly when imported in page route.
expect($('#widget').text()).toBe('widget')
})

it('should handle required segments root as not found', async () => {
Expand Down
20 changes: 19 additions & 1 deletion test/unit/find-page-file.test.ts
@@ -1,5 +1,8 @@
/* eslint-env jest */
import { findPageFile } from 'next/dist/server/lib/find-page-file'
import {
findPageFile,
isLayoutsLeafPage,
} from 'next/dist/server/lib/find-page-file'
import { normalizePagePath } from 'next/dist/shared/lib/page-path/normalize-page-path'

import { join } from 'path'
Expand All @@ -26,3 +29,18 @@ describe('findPageFile', () => {
expect(result).toMatch(/^[\\/]prefered\.js/)
})
})

describe('isLayoutsLeafPage', () => {
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)
})

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