Skip to content

Commit

Permalink
Turbopack build: Ensure Pages Router manifests are loaded with buildId (
Browse files Browse the repository at this point in the history
#67317)

1. The build manifest incorrectly held a hardcoded `/development/` which
doesn't work in production where buildId is unique.
2. Implemented `__rewrites` in the build manifest, this was a TODO in
the codebase. After fixing 1. it highlighted that tests related to it
were incorrectly passing. Made sure those tests still pass by
implementing the feature.
3. Implemented middleware matchers on the client-side, this was missing
for production and was highlighted by fixing 1. as well. Had to take a
bit different approach for Turbopack as in the webpack case we're
parsing the entrypoint manually in Next.js which is not ideal. Planning
to change this to be written into the `_buildManifest` but for now it's
a separate file in the same way as we handle it for development.
4. Made sure that rewrites to an external url are not in the manifest,
similar case, highlighted test wasn't passing by fixing 1. then fixed
it.


<!-- Thanks for opening a PR! Your contribution is much appreciated.
To make sure your PR is handled as smoothly as possible we request that
you follow the checklist sections below.
Choose the right checklist for the change(s) that you're making:

## For Contributors

### Improving Documentation

- Run `pnpm prettier-fix` to fix formatting issues before opening the
PR.
- Read the Docs Contribution Guide to ensure your contribution follows
the docs guidelines:
https://nextjs.org/docs/community/contribution-guide

### Adding or Updating Examples

- The "examples guidelines" are followed from our contributing doc
https://github.com/vercel/next.js/blob/canary/contributing/examples/adding-examples.md
- Make sure the linting passes by running `pnpm build && pnpm lint`. See
https://github.com/vercel/next.js/blob/canary/contributing/repository/linting.md

### Fixing a bug

- Related issues linked using `fixes #number`
- Tests added. See:
https://github.com/vercel/next.js/blob/canary/contributing/core/testing.md#writing-tests-for-nextjs
- Errors have a helpful link attached, see
https://github.com/vercel/next.js/blob/canary/contributing.md

### Adding a feature

- Implements an existing feature request or RFC. Make sure the feature
request has been accepted for implementation before opening a PR. (A
discussion must be opened, see
https://github.com/vercel/next.js/discussions/new?category=ideas)
- Related issues/discussions are linked using `fixes #number`
- e2e tests added
(https://github.com/vercel/next.js/blob/canary/contributing/core/testing.md#writing-tests-for-nextjs)
- Documentation added
- Telemetry added. In case of a feature if it's used or not.
- Errors have a helpful link attached, see
https://github.com/vercel/next.js/blob/canary/contributing.md


## For Maintainers

- Minimal description (aim for explaining to someone not on the team to
understand the PR)
- When linking to a Slack thread, you might want to share details of the
conclusion
- Link both the Linear (Fixes NEXT-xxx) and the GitHub issues
- Add review comments if necessary to explain to the reviewer the logic
behind a change

### What?

### Why?

### How?

Closes NEXT-
Fixes #

-->
  • Loading branch information
timneutkens committed Jul 7, 2024
1 parent d7c0895 commit 0177545
Show file tree
Hide file tree
Showing 11 changed files with 181 additions and 84 deletions.
26 changes: 12 additions & 14 deletions packages/next/src/build/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1353,7 +1353,7 @@ export default async function build(
distDir,
fetchCacheKeyPrefix: config.experimental.fetchCacheKeyPrefix,
hasRewrites,
// TODO: Implement
// Implemented separately in Turbopack, doesn't have to be passed here.
middlewareMatchers: undefined,
}),
buildId: NextBuildContext.buildId!,
Expand Down Expand Up @@ -1404,13 +1404,6 @@ export default async function build(
encryptionKey,
})

// TODO: implement this
const emptyRewritesObjToBeImplemented = {
beforeFiles: [],
afterFiles: [],
fallback: [],
}

const entrypointsResult = await entrypointsSubscription.next()
if (entrypointsResult.done) {
throw new Error('Turbopack did not return any entrypoints')
Expand Down Expand Up @@ -1442,7 +1435,8 @@ export default async function build(
currentEntryIssues,
manifestLoader,
nextConfig: config,
rewrites: emptyRewritesObjToBeImplemented,
devRewrites: undefined,
productionRewrites: customRoutes.rewrites,
logErrors: false,
})

Expand Down Expand Up @@ -1477,7 +1471,8 @@ export default async function build(
currentEntryIssues,
entrypoints: currentEntrypoints,
manifestLoader,
rewrites: emptyRewritesObjToBeImplemented,
devRewrites: undefined,
productionRewrites: customRoutes.rewrites,
logErrors: false,
})
)
Expand All @@ -1493,7 +1488,8 @@ export default async function build(
currentEntryIssues,
entrypoints: currentEntrypoints,
manifestLoader,
rewrites: emptyRewritesObjToBeImplemented,
devRewrites: undefined,
productionRewrites: customRoutes.rewrites,
logErrors: false,
})
)
Expand All @@ -1504,15 +1500,17 @@ export default async function build(
currentEntryIssues,
entrypoints: currentEntrypoints,
manifestLoader,
rewrites: emptyRewritesObjToBeImplemented,
devRewrites: undefined,
productionRewrites: customRoutes.rewrites,
logErrors: false,
})
)
await Promise.all(promises)

await manifestLoader.writeManifests({
rewrites: emptyRewritesObjToBeImplemented,
pageEntrypoints: currentEntrypoints.page,
devRewrites: undefined,
productionRewrites: customRoutes.rewrites,
entrypoints: currentEntrypoints,
})

const errors: {
Expand Down
14 changes: 10 additions & 4 deletions packages/next/src/build/webpack/plugins/build-manifest-plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -77,9 +77,15 @@ export function normalizeRewritesForBuildManifest(
rewrites: CustomRoutes['rewrites']
): CustomRoutes['rewrites'] {
return {
afterFiles: rewrites.afterFiles?.map((item) => normalizeRewrite(item)),
beforeFiles: rewrites.beforeFiles?.map((item) => normalizeRewrite(item)),
fallback: rewrites.fallback?.map((item) => normalizeRewrite(item)),
afterFiles: rewrites.afterFiles
?.map(processRoute)
?.map((item) => normalizeRewrite(item)),
beforeFiles: rewrites.beforeFiles
?.map(processRoute)
?.map((item) => normalizeRewrite(item)),
fallback: rewrites.fallback
?.map(processRoute)
?.map((item) => normalizeRewrite(item)),
}
}

Expand Down Expand Up @@ -143,7 +149,7 @@ export const processRoute = (r: Rewrite) => {

// omit external rewrite destinations since these aren't
// handled client-side
if (!rewrite.destination.startsWith('/')) {
if (!rewrite?.destination?.startsWith('/')) {
delete (rewrite as any).destination
}
return rewrite
Expand Down
6 changes: 5 additions & 1 deletion packages/next/src/build/webpack/plugins/define-env-plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,11 @@ export function getDefineEnv({
'process.env.__NEXT_AFTER': config.experimental.after ?? false,
'process.env.NEXT_DEPLOYMENT_ID': config.deploymentId || false,
'process.env.__NEXT_FETCH_CACHE_KEY_PREFIX': fetchCacheKeyPrefix ?? '',
'process.env.__NEXT_MIDDLEWARE_MATCHERS': middlewareMatchers ?? [],
...(isTurbopack
? {}
: {
'process.env.__NEXT_MIDDLEWARE_MATCHERS': middlewareMatchers ?? [],
}),
'process.env.__NEXT_MANUAL_CLIENT_BASE_PATH':
config.experimental.manualClientBasePath ?? false,
'process.env.__NEXT_CLIENT_ROUTER_DYNAMIC_STALETIME': JSON.stringify(
Expand Down
36 changes: 33 additions & 3 deletions packages/next/src/client/page-loader.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ import { removeTrailingSlash } from '../shared/lib/router/utils/remove-trailing-
import { createRouteLoader, getClientBuildManifest } from './route-loader'
import {
DEV_CLIENT_PAGES_MANIFEST,
DEV_MIDDLEWARE_MANIFEST,
DEV_CLIENT_MIDDLEWARE_MANIFEST,
TURBOPACK_CLIENT_MIDDLEWARE_MANIFEST,
} from '../shared/lib/constants'

declare global {
Expand Down Expand Up @@ -85,12 +86,41 @@ export default class PageLoader {
}

getMiddleware() {
if (process.env.NODE_ENV === 'production') {
// Webpack production
if (
process.env.NODE_ENV === 'production' &&
process.env.__NEXT_MIDDLEWARE_MATCHERS
) {
const middlewareMatchers = process.env.__NEXT_MIDDLEWARE_MATCHERS
window.__MIDDLEWARE_MATCHERS = middlewareMatchers
? (middlewareMatchers as any as MiddlewareMatcher[])
: undefined
return window.__MIDDLEWARE_MATCHERS
// Turbopack production
} else if (process.env.NODE_ENV === 'production') {
if (window.__MIDDLEWARE_MATCHERS) {
return window.__MIDDLEWARE_MATCHERS
} else {
if (!this.promisedMiddlewareMatchers) {
// TODO: Decide what should happen when fetching fails instead of asserting
// @ts-ignore
this.promisedMiddlewareMatchers = fetch(
`${this.assetPrefix}/_next/static/${this.buildId}/${TURBOPACK_CLIENT_MIDDLEWARE_MANIFEST}`,
{ credentials: 'same-origin' }
)
.then((res) => res.json())
.then((matchers: MiddlewareMatcher[]) => {
window.__MIDDLEWARE_MATCHERS = matchers
return matchers
})
.catch((err) => {
console.log(`Failed to fetch _devMiddlewareManifest`, err)
})
}
// TODO Remove this assertion as this could be undefined
return this.promisedMiddlewareMatchers!
}
// Development both Turbopack and Webpack
} else {
if (window.__DEV_MIDDLEWARE_MATCHERS) {
return window.__DEV_MIDDLEWARE_MATCHERS
Expand All @@ -99,7 +129,7 @@ export default class PageLoader {
// TODO: Decide what should happen when fetching fails instead of asserting
// @ts-ignore
this.promisedMiddlewareMatchers = fetch(
`${this.assetPrefix}/_next/static/${this.buildId}/${DEV_MIDDLEWARE_MANIFEST}`,
`${this.assetPrefix}/_next/static/${this.buildId}/${DEV_CLIENT_MIDDLEWARE_MANIFEST}`,
{ credentials: 'same-origin' }
)
.then((res) => res.json())
Expand Down
14 changes: 9 additions & 5 deletions packages/next/src/server/dev/hot-reloader-turbopack.ts
Original file line number Diff line number Diff line change
Expand Up @@ -462,7 +462,8 @@ export async function createHotReloaderTurbopack(
currentEntryIssues,
manifestLoader,
nextConfig: opts.nextConfig,
rewrites: opts.fsChecker.rewrites,
devRewrites: opts.fsChecker.rewrites,
productionRewrites: undefined,
logErrors: true,

dev: {
Expand Down Expand Up @@ -800,7 +801,8 @@ export async function createHotReloaderTurbopack(
currentEntryIssues,
entrypoints: currentEntrypoints,
manifestLoader,
rewrites: opts.fsChecker.rewrites,
devRewrites: opts.fsChecker.rewrites,
productionRewrites: undefined,
logErrors: true,

hooks: {
Expand Down Expand Up @@ -861,7 +863,8 @@ export async function createHotReloaderTurbopack(
entrypoints: currentEntrypoints,
manifestLoader,
readyIds,
rewrites: opts.fsChecker.rewrites,
devRewrites: opts.fsChecker.rewrites,
productionRewrites: undefined,
logErrors: true,

hooks: {
Expand All @@ -887,8 +890,9 @@ export async function createHotReloaderTurbopack(
// Write empty manifests
await currentEntriesHandling
await manifestLoader.writeManifests({
rewrites: opts.fsChecker.rewrites,
pageEntrypoints: currentEntrypoints.page,
devRewrites: opts.fsChecker.rewrites,
productionRewrites: undefined,
entrypoints: currentEntrypoints,
})

async function handleProjectUpdates() {
Expand Down
54 changes: 34 additions & 20 deletions packages/next/src/server/dev/turbopack-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import {
import type ws from 'next/dist/compiled/ws'
import isInternal from '../../shared/lib/is-internal'
import { isMetadataRoute } from '../../lib/metadata/is-metadata-route'
import type { CustomRoutes } from '../../lib/load-custom-routes'

export async function getTurbopackJsConfig(
dir: string,
Expand Down Expand Up @@ -328,7 +329,8 @@ export async function handleRouteType({
entrypoints,
manifestLoader,
readyIds,
rewrites,
devRewrites,
productionRewrites,
hooks,
logErrors,
}: {
Expand All @@ -340,7 +342,8 @@ export async function handleRouteType({
currentEntryIssues: EntryIssuesMap
entrypoints: Entrypoints
manifestLoader: TurbopackManifestLoader
rewrites: SetupOpts['fsChecker']['rewrites']
devRewrites: SetupOpts['fsChecker']['rewrites'] | undefined
productionRewrites: CustomRoutes['rewrites'] | undefined
logErrors: boolean

readyIds?: ReadyIds // dev
Expand Down Expand Up @@ -402,8 +405,9 @@ export async function handleRouteType({
await manifestLoader.loadLoadableManifest(page, 'pages')

await manifestLoader.writeManifests({
rewrites,
pageEntrypoints: entrypoints.page,
devRewrites,
productionRewrites,
entrypoints,
})

processIssues(
Expand Down Expand Up @@ -460,8 +464,9 @@ export async function handleRouteType({
await manifestLoader.loadLoadableManifest(page, 'pages')

await manifestLoader.writeManifests({
rewrites,
pageEntrypoints: entrypoints.page,
devRewrites,
productionRewrites,
entrypoints,
})

processIssues(currentEntryIssues, key, writtenEndpoint, true, logErrors)
Expand Down Expand Up @@ -504,8 +509,9 @@ export async function handleRouteType({
await manifestLoader.loadLoadableManifest(page, 'app')
await manifestLoader.loadFontManifest(page, 'app')
await manifestLoader.writeManifests({
rewrites,
pageEntrypoints: entrypoints.page,
devRewrites,
productionRewrites,
entrypoints,
})

processIssues(currentEntryIssues, key, writtenEndpoint, dev, logErrors)
Expand All @@ -529,8 +535,9 @@ export async function handleRouteType({
}

await manifestLoader.writeManifests({
rewrites,
pageEntrypoints: entrypoints.page,
devRewrites,
productionRewrites,
entrypoints,
})
processIssues(currentEntryIssues, key, writtenEndpoint, true, logErrors)

Expand Down Expand Up @@ -685,7 +692,8 @@ export async function handleEntrypoints({
currentEntryIssues,
manifestLoader,
nextConfig,
rewrites,
devRewrites,
productionRewrites,
logErrors,
dev,
}: {
Expand All @@ -696,7 +704,8 @@ export async function handleEntrypoints({
currentEntryIssues: EntryIssuesMap
manifestLoader: TurbopackManifestLoader
nextConfig: NextConfigComplete
rewrites: SetupOpts['fsChecker']['rewrites']
devRewrites: SetupOpts['fsChecker']['rewrites'] | undefined
productionRewrites: CustomRoutes['rewrites'] | undefined
logErrors: boolean

dev?: HandleEntrypointsDevOpts
Expand Down Expand Up @@ -784,8 +793,9 @@ export async function handleEntrypoints({
'instrumentation'
)
await manifestLoader.writeManifests({
rewrites: rewrites,
pageEntrypoints: currentEntrypoints.page,
devRewrites,
productionRewrites,
entrypoints: currentEntrypoints,
})

if (dev) {
Expand Down Expand Up @@ -842,8 +852,9 @@ export async function handleEntrypoints({
dev.serverFields.middleware
)
await manifestLoader.writeManifests({
rewrites: rewrites,
pageEntrypoints: currentEntrypoints.page,
devRewrites,
productionRewrites,
entrypoints: currentEntrypoints,
})

finishBuilding?.()
Expand Down Expand Up @@ -935,15 +946,17 @@ export async function handlePagesErrorRoute({
currentEntryIssues,
entrypoints,
manifestLoader,
rewrites,
devRewrites,
productionRewrites,
logErrors,

hooks,
}: {
currentEntryIssues: EntryIssuesMap
entrypoints: Entrypoints
manifestLoader: TurbopackManifestLoader
rewrites: SetupOpts['fsChecker']['rewrites']
devRewrites: SetupOpts['fsChecker']['rewrites'] | undefined
productionRewrites: CustomRoutes['rewrites'] | undefined
logErrors: boolean

hooks?: HandleRouteTypeHooks // dev
Expand Down Expand Up @@ -993,8 +1006,9 @@ export async function handlePagesErrorRoute({
await manifestLoader.loadFontManifest('_error')

await manifestLoader.writeManifests({
rewrites,
pageEntrypoints: entrypoints.page,
devRewrites,
productionRewrites,
entrypoints,
})
}

Expand Down
Loading

0 comments on commit 0177545

Please sign in to comment.