From 7e7d7bbca6700daf9ad8f40c7575b134c71e8cc7 Mon Sep 17 00:00:00 2001 From: JJ Kasper Date: Tue, 26 Apr 2022 14:12:29 -0500 Subject: [PATCH] Fix incorrect asPath with fallback rewrite in minimal mode (#36463) This continues off of the change in https://github.com/vercel/next.js/pull/36368 and ensures a fallback rewrite does not influence the `asPath` as these are only matched when a filesystem or dynamic route aren't matched. ## Bug - [x] Related issues linked using `fixes #number` - [x] Integration tests added - [ ] Errors have helpful link attached, see `contributing.md` x-ref: https://github.com/vercel/next.js/pull/36368 --- .../loaders/next-serverless-loader/index.ts | 32 +++++------- .../loaders/next-serverless-loader/utils.ts | 44 ++++++++++++++-- packages/next/server/base-server.ts | 18 ++++--- test/production/required-server-files.test.ts | 52 ++++++++++++++----- .../pages/[slug]/index.js | 10 +++- 5 files changed, 108 insertions(+), 48 deletions(-) diff --git a/packages/next/build/webpack/loaders/next-serverless-loader/index.ts b/packages/next/build/webpack/loaders/next-serverless-loader/index.ts index dfcf7e26c58ea94..0141539a841db06 100644 --- a/packages/next/build/webpack/loaders/next-serverless-loader/index.ts +++ b/packages/next/build/webpack/loaders/next-serverless-loader/index.ts @@ -105,19 +105,15 @@ const nextServerlessLoader: webpack.loader.Loader = function () { import { getApiHandler } from 'next/dist/build/webpack/loaders/next-serverless-loader/api-handler' - const combinedRewrites = Array.isArray(routesManifest.rewrites) - ? routesManifest.rewrites - : [] - - if (!Array.isArray(routesManifest.rewrites)) { - combinedRewrites.push(...routesManifest.rewrites.beforeFiles) - combinedRewrites.push(...routesManifest.rewrites.afterFiles) - combinedRewrites.push(...routesManifest.rewrites.fallback) - } + const rewrites = Array.isArray(routesManifest.rewrites) + ? { + afterFiles: routesManifest.rewrites + } + : routesManifest.rewrites const apiHandler = getApiHandler({ pageModule: require(${stringifyRequest(this, absolutePagePath)}), - rewrites: combinedRewrites, + rewrites: rewrites, i18n: ${i18n || 'undefined'}, page: "${page}", basePath: "${basePath}", @@ -166,15 +162,11 @@ const nextServerlessLoader: webpack.loader.Loader = function () { export let config = compMod['confi' + 'g'] || (compMod.then && compMod.then(mod => mod['confi' + 'g'])) || {} export const _app = App - const combinedRewrites = Array.isArray(routesManifest.rewrites) - ? routesManifest.rewrites - : [] - - if (!Array.isArray(routesManifest.rewrites)) { - combinedRewrites.push(...routesManifest.rewrites.beforeFiles) - combinedRewrites.push(...routesManifest.rewrites.afterFiles) - combinedRewrites.push(...routesManifest.rewrites.fallback) - } + const rewrites = Array.isArray(routesManifest.rewrites) + ? { + afterFiles: routesManifest.rewrites + } + : routesManifest.rewrites const { renderReqToHTML, render } = getPageHandler({ pageModule: compMod, @@ -202,7 +194,7 @@ const nextServerlessLoader: webpack.loader.Loader = function () { buildManifest, reactLoadableManifest, - rewrites: combinedRewrites, + rewrites: rewrites, i18n: ${i18n || 'undefined'}, page: "${page}", buildId: "${buildId}", diff --git a/packages/next/build/webpack/loaders/next-serverless-loader/utils.ts b/packages/next/build/webpack/loaders/next-serverless-loader/utils.ts index 338e111e0573d2e..391675d76b94467 100644 --- a/packages/next/build/webpack/loaders/next-serverless-loader/utils.ts +++ b/packages/next/build/webpack/loaders/next-serverless-loader/utils.ts @@ -51,7 +51,11 @@ export type ServerlessHandlerCtx = { buildManifest?: BuildManifest reactLoadableManifest?: any basePath: string - rewrites: Rewrite[] + rewrites: { + fallback?: Rewrite[] + afterFiles?: Rewrite[] + beforeFiles?: Rewrite[] + } pageIsDynamic: boolean generateEtags: boolean distDir: string @@ -92,8 +96,13 @@ export function getUtils({ parsedUrl: UrlWithParsedQuery ) { const rewriteParams = {} + let fsPathname = parsedUrl.pathname + + const matchesPage = () => { + return fsPathname === page || dynamicRouteMatcher?.(fsPathname) + } - for (const rewrite of rewrites) { + const checkRewrite = (rewrite: Rewrite): boolean => { const matcher = getCustomRouteMatcher(rewrite.source) let params = matcher(parsedUrl.pathname) @@ -115,13 +124,18 @@ export function getUtils({ query: parsedUrl.query, }) + // if the rewrite destination is external break rewrite chain + if (parsedDestination.protocol) { + return true + } + Object.assign(rewriteParams, destQuery, params) Object.assign(parsedUrl.query, parsedDestination.query) delete (parsedDestination as any).query Object.assign(parsedUrl, parsedDestination) - let fsPathname = parsedUrl.pathname + fsPathname = parsedUrl.pathname if (basePath) { fsPathname = @@ -139,7 +153,7 @@ export function getUtils({ } if (fsPathname === page) { - break + return true } if (pageIsDynamic && dynamicRouteMatcher) { @@ -149,12 +163,32 @@ export function getUtils({ ...parsedUrl.query, ...dynamicParams, } - break + return true } } } + return false + } + + for (const rewrite of rewrites.beforeFiles || []) { + checkRewrite(rewrite) } + if (fsPathname !== page) { + let finished = false + + for (const rewrite of rewrites.afterFiles || []) { + finished = checkRewrite(rewrite) + if (finished) break + } + + if (!finished && !matchesPage()) { + for (const rewrite of rewrites.fallback || []) { + finished = checkRewrite(rewrite) + if (finished) break + } + } + } return rewriteParams } diff --git a/packages/next/server/base-server.ts b/packages/next/server/base-server.ts index 3bde9a187d47cd0..4ed04501fcb51bc 100644 --- a/packages/next/server/base-server.ts +++ b/packages/next/server/base-server.ts @@ -8,7 +8,6 @@ import type { MiddlewareManifest } from '../build/webpack/plugins/middleware-plu import type { NextConfig, NextConfigComplete } from './config-shared' import type { NextParsedUrlQuery, NextUrlWithParsedQuery } from './request-meta' import type { ParsedUrlQuery } from 'querystring' -import type { Rewrite } from '../lib/load-custom-routes' import type { RenderOpts, RenderOptsPartial } from './render' import type { ResponseCacheEntry, ResponseCacheValue } from './response-cache' import type { UrlWithParsedQuery } from 'url' @@ -479,19 +478,22 @@ export default abstract class Server { srcPathname = denormalizePagePath(srcPathname) } - const pageIsDynamic = isDynamicRoute(srcPathname) - const combinedRewrites: Rewrite[] = [] - - combinedRewrites.push(...this.customRoutes.rewrites.beforeFiles) - combinedRewrites.push(...this.customRoutes.rewrites.afterFiles) - combinedRewrites.push(...this.customRoutes.rewrites.fallback) + if (!isDynamicRoute(srcPathname) && !this.hasPage(srcPathname)) { + for (const dynamicRoute of this.dynamicRoutes || []) { + if (dynamicRoute.match(srcPathname)) { + srcPathname = dynamicRoute.page + break + } + } + } + const pageIsDynamic = isDynamicRoute(srcPathname) const utils = getUtils({ pageIsDynamic, page: srcPathname, i18n: this.nextConfig.i18n, basePath: this.nextConfig.basePath, - rewrites: combinedRewrites, + rewrites: this.customRoutes.rewrites, }) try { diff --git a/test/production/required-server-files.test.ts b/test/production/required-server-files.test.ts index b1bdbba48aa5464..405e87c26cc81df 100644 --- a/test/production/required-server-files.test.ts +++ b/test/production/required-server-files.test.ts @@ -49,20 +49,29 @@ describe('should set-up next', () => { outputStandalone: true, }, async rewrites() { - return [ - { - source: '/some-catch-all/:path*', - destination: '/', - }, - { - source: '/to-dynamic/post-2', - destination: '/dynamic/post-2?hello=world', - }, - { - source: '/to-dynamic/:path', - destination: '/dynamic/:path', - }, - ] + return { + beforeFiles: [], + fallback: [ + { + source: '/an-ssg-path', + destination: '/hello.txt', + }, + ], + afterFiles: [ + { + source: '/some-catch-all/:path*', + destination: '/', + }, + { + source: '/to-dynamic/post-2', + destination: '/dynamic/post-2?hello=world', + }, + { + source: '/to-dynamic/:path', + destination: '/dynamic/:path', + }, + ], + } }, }, }) @@ -691,6 +700,7 @@ describe('should set-up next', () => { expect(res.status).toBe(200) const $ = cheerio.load(await res.text()) expect($('#resolved-url').text()).toBe('/dynamic/post-2') + expect(JSON.parse($('#router').text()).asPath).toBe('/to-dynamic/post-2') }) it('should have correct resolvedUrl from dynamic route', async () => { @@ -876,6 +886,20 @@ describe('should set-up next', () => { expect($('#slug-page').text()).toBe('[slug] page') }) + it('should have correct asPath on dynamic SSG page correctly', async () => { + const res = await fetchViaHTTP(appPort, '/an-ssg-path', undefined, { + headers: { + 'x-matched-path': '/[slug]', + }, + redirect: 'manual', + }) + + const html = await res.text() + const $ = cheerio.load(html) + expect($('#slug-page').text()).toBe('[slug] page') + expect(JSON.parse($('#router').text()).asPath).toBe('/an-ssg-path') + }) + it('should copy and read .env file', async () => { const res = await fetchViaHTTP(appPort, '/api/env') diff --git a/test/production/required-server-files/pages/[slug]/index.js b/test/production/required-server-files/pages/[slug]/index.js index 9898cbe9722d9ad..7ea16645298ef7e 100644 --- a/test/production/required-server-files/pages/[slug]/index.js +++ b/test/production/required-server-files/pages/[slug]/index.js @@ -1,3 +1,5 @@ +import { useRouter } from 'next/router' + export const getStaticProps = () => { return { props: { @@ -14,5 +16,11 @@ export const getStaticPaths = () => { } export default function Page(props) { - return

[slug] page

+ const router = useRouter() + return ( + <> +

[slug] page

+

{JSON.stringify(router)}

+ + ) }