Skip to content

Commit

Permalink
Fix incorrect asPath with fallback rewrite in minimal mode (#36463)
Browse files Browse the repository at this point in the history
This continues off of the change in #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: #36368
  • Loading branch information
ijjk committed Apr 26, 2022
1 parent d5e767b commit 7e7d7bb
Show file tree
Hide file tree
Showing 5 changed files with 108 additions and 48 deletions.
32 changes: 12 additions & 20 deletions packages/next/build/webpack/loaders/next-serverless-loader/index.ts
Expand Up @@ -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}",
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -202,7 +194,7 @@ const nextServerlessLoader: webpack.loader.Loader = function () {
buildManifest,
reactLoadableManifest,
rewrites: combinedRewrites,
rewrites: rewrites,
i18n: ${i18n || 'undefined'},
page: "${page}",
buildId: "${buildId}",
Expand Down
Expand Up @@ -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
Expand Down Expand Up @@ -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)

Expand All @@ -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 =
Expand All @@ -139,7 +153,7 @@ export function getUtils({
}

if (fsPathname === page) {
break
return true
}

if (pageIsDynamic && dynamicRouteMatcher) {
Expand All @@ -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
}

Expand Down
18 changes: 10 additions & 8 deletions packages/next/server/base-server.ts
Expand Up @@ -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'
Expand Down Expand Up @@ -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 {
Expand Down
52 changes: 38 additions & 14 deletions test/production/required-server-files.test.ts
Expand Up @@ -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',
},
],
}
},
},
})
Expand Down Expand Up @@ -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 () => {
Expand Down Expand Up @@ -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')

Expand Down
10 changes: 9 additions & 1 deletion test/production/required-server-files/pages/[slug]/index.js
@@ -1,3 +1,5 @@
import { useRouter } from 'next/router'

export const getStaticProps = () => {
return {
props: {
Expand All @@ -14,5 +16,11 @@ export const getStaticPaths = () => {
}

export default function Page(props) {
return <p id="slug-page">[slug] page</p>
const router = useRouter()
return (
<>
<p id="slug-page">[slug] page</p>
<p id="router">{JSON.stringify(router)}</p>
</>
)
}

0 comments on commit 7e7d7bb

Please sign in to comment.