Skip to content

Commit

Permalink
Treat non page file as non route under app dir (#39976)
Browse files Browse the repository at this point in the history
## Bug

Adding a components folder inside a catch-all page triggers the error: "Catch-all must be the last part of the URL", having the component outside works, for example `app/[...slug]/components/*.tsx` fails but `app/components/*.tsx` works as expected.

### Fix

The reason it errors because they're treated as route and inserted into the url node tree where they shouldn't be treated in this way. Adding a helper to skip collecting and normailizing every file as route for app dir, but only do it for page files

Co-authored-by: JJ Kasper <jj@jjsweb.site>
  • Loading branch information
huozhi and ijjk committed Aug 26, 2022
1 parent 7397be7 commit 2407ab2
Show file tree
Hide file tree
Showing 6 changed files with 46 additions and 5 deletions.
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,
// 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)
}
@@ -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)
})
})

0 comments on commit 2407ab2

Please sign in to comment.