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.

Sign up for GitHub

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

Add support for optional catchall with new router #38444

Merged
merged 13 commits into from
Jul 8, 2022
Merged

Add support for optional catchall with new router #38444

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
0ddcedd
Add support for catchall with new router
timneutkens Jul 8, 2022
1672a77
Add missing brackets
timneutkens Jul 8, 2022
632acc5
Update app-render.tsx
timneutkens Jul 8, 2022
29e95c0
Merge branch 'canary' into add/catchall-new-router
kodiakhq[bot] Jul 8, 2022
0e899d1
Update base-server.ts
timneutkens Jul 8, 2022
61bbdb1
Rename root -> app
timneutkens Jul 8, 2022
28886a1
Handle params being an array
timneutkens Jul 8, 2022
107c6e5
Remove _root condition
timneutkens Jul 8, 2022
5eb122b
Merge branch 'canary' of github.com:vercel/next.js into add/catchall-…
timneutkens Jul 8, 2022
aaffd48
Add support for optional catch all routes
timneutkens Jul 8, 2022
bc45309
Handle empty optional catchall
timneutkens Jul 8, 2022
4c81c50
Simplify
timneutkens Jul 8, 2022
1f4a65d
Merge branch 'canary' into add/catchall-new-router
kodiakhq[bot] Jul 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions packages/next/build/webpack/plugins/pages-manifest-plugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ export type PagesManifest = { [page: string]: string }

let edgeServerPages = {}
let nodeServerPages = {}
let edgeServerRootPaths = {}
let nodeServerRootPaths = {}
let edgeServerAppPaths = {}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All changes in this file are just renames.

let nodeServerAppPaths = {}

// This plugin creates a pages-manifest.json from page entrypoints.
// This is used for mapping paths like `/` to `.next/server/static/<buildid>/pages/index.js` when doing SSR
Expand Down Expand Up @@ -42,7 +42,7 @@ export default class PagesManifestPlugin implements webpack.Plugin {
createAssets(compilation: any, assets: any) {
const entrypoints = compilation.entrypoints
const pages: PagesManifest = {}
const rootPaths: PagesManifest = {}
const appPaths: PagesManifest = {}

for (const entrypoint of entrypoints.values()) {
const pagePath = getRouteFromEntrypoint(
Expand Down Expand Up @@ -78,7 +78,7 @@ export default class PagesManifestPlugin implements webpack.Plugin {
file = normalizePathSep(file)

if (entrypoint.name.startsWith('app/')) {
rootPaths[pagePath] = file
appPaths[pagePath] = file
} else {
pages[pagePath] = file
}
Expand All @@ -88,10 +88,10 @@ export default class PagesManifestPlugin implements webpack.Plugin {
// we need to merge both pages to generate the full manifest.
if (this.isEdgeRuntime) {
edgeServerPages = pages
edgeServerRootPaths = rootPaths
edgeServerAppPaths = appPaths
} else {
nodeServerPages = pages
nodeServerRootPaths = rootPaths
nodeServerAppPaths = appPaths
}

assets[
Expand All @@ -113,8 +113,8 @@ export default class PagesManifestPlugin implements webpack.Plugin {
] = new sources.RawSource(
JSON.stringify(
{
...edgeServerRootPaths,
...nodeServerRootPaths,
...edgeServerAppPaths,
...nodeServerAppPaths,
},
null,
2
Expand Down
6 changes: 4 additions & 2 deletions packages/next/server/app-render.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -394,10 +394,12 @@ export async function renderToHTML(

return {
param: segmentParam,
// @ts-expect-error TODO: handle case where value is an array
value:
// TODO: this should only read from `pathParams`. There's an inconsistency where `query` holds params currently which has to be fixed.
pathParams[segmentParam] ??
(Array.isArray(pathParams[segmentParam])
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missed this in the first PR. The pathParams value can also be an array.

? // @ts-expect-error TODO: handle case where value is an array
pathParams[segmentParam].join('/')
: pathParams[segmentParam]) ??
(Array.isArray(query[segmentParam])
? // @ts-expect-error TODO: handle case where value is an array
query[segmentParam].join('/')
Expand Down
26 changes: 4 additions & 22 deletions packages/next/server/base-server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import { format as formatUrl, parse as parseUrl } from 'url'
import { getRedirectStatus } from '../lib/load-custom-routes'
import {
NEXT_BUILTIN_DOCUMENT,
NEXT_CLIENT_SSR_ENTRY_SUFFIX,
SERVERLESS_DIRECTORY,
SERVER_DIRECTORY,
STATIC_STATUS_PAGES,
Expand Down Expand Up @@ -1026,33 +1027,14 @@ export default abstract class Server<ServerOptions extends Options = Options> {
const appPathRoutes: Record<string, string> = {}

Object.keys(this.appPathsManifest || {}).forEach((entry) => {
if (entry.endsWith(NEXT_CLIENT_SSR_ENTRY_SUFFIX)) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The __sc_client__ entries are included in the manifest for routing but it caused them to be routable here. Only reason I noticed was that [...slug] has a check for if it's the last segment or not and then throws.

return
}
appPathRoutes[normalizeAppPath(entry) || '/'] = entry
})
return appPathRoutes
}

protected getAppPathLayouts(pathname: string): string[] {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is no longer used so it was dead code.

const layoutPaths: string[] = []

if (this.appPathRoutes) {
const paths = Object.values(this.appPathRoutes)
const parts = pathname.split('/').filter(Boolean)

for (let i = 1; i < parts.length; i++) {
const layoutPath = `/${parts.slice(0, i).join('/')}/layout`

if (paths.includes(layoutPath)) {
layoutPaths.push(layoutPath)
}
}

if (this.appPathRoutes['/layout']) {
layoutPaths.unshift('/layout')
}
}
return layoutPaths
}

protected async run(
req: BaseNextRequest,
res: BaseNextResponse,
Expand Down
3 changes: 0 additions & 3 deletions packages/next/server/require.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,6 @@ export function getPagePath(
let rootPathsManifest: undefined | PagesManifest

if (appDirEnabled) {
if (page === '/_root') {
return join(serverBuildPath, 'root.js')
}
rootPathsManifest = require(join(serverBuildPath, APP_PATHS_MANIFEST))
}
const pagesManifest = require(join(
Expand Down