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.

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

Ensure resolvedUrl is correct with fallback rewrites #37932

Merged
merged 3 commits into from Jun 22, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -28,6 +28,7 @@ import { denormalizePagePath } from '../../../../shared/lib/page-path/denormaliz
import cookie from 'next/dist/compiled/cookie'
import { TEMPORARY_REDIRECT_STATUS } from '../../../../shared/lib/constants'
import { addRequestMeta } from '../../../../server/request-meta'
import { removeTrailingSlash } from '../../../../shared/lib/router/utils/remove-trailing-slash'

export const vercelHeader = 'x-vercel-id'

Expand Down Expand Up @@ -98,7 +99,11 @@ export function getUtils({
let fsPathname = parsedUrl.pathname

const matchesPage = () => {
return fsPathname === page || dynamicRouteMatcher?.(fsPathname)
const fsPathnameNoSlash = removeTrailingSlash(fsPathname || '')
return (
fsPathnameNoSlash === removeTrailingSlash(page) ||
dynamicRouteMatcher?.(fsPathnameNoSlash)
)
}

const checkRewrite = (rewrite: Rewrite): boolean => {
Expand Down
5 changes: 5 additions & 0 deletions packages/next/server/base-server.ts
Expand Up @@ -1341,6 +1341,11 @@ export default abstract class Server<ServerOptions extends Options = Options> {
let resolvedUrlPathname =
getRequestMeta(req, '_nextRewroteUrl') || urlPathname

if (isSSG && this.minimalMode && req.headers['x-matched-path']) {
// the url value is already correct when the matched-path header is set
resolvedUrlPathname = urlPathname
}

urlPathname = removeTrailingSlash(urlPathname)
resolvedUrlPathname = normalizeLocalePath(
removeTrailingSlash(resolvedUrlPathname),
Expand Down
32 changes: 32 additions & 0 deletions test/production/minimal-mode-response-cache/app/next.config.js
@@ -0,0 +1,32 @@
module.exports = {
experimental: {
outputStandalone: true,
},
trailingSlash: true,
rewrites() {
return {
beforeFiles: [
{
source: '/news/:path/',
destination: '/news/:path*/',
},
],
afterFiles: [
{
source: '/somewhere',
destination: '/',
},
],
fallback: [
{
source: '/:path*',
destination: '/:path*',
},
{
source: '/(.*)',
destination: '/seo-redirects',
},
],
}
},
}
@@ -0,0 +1,32 @@
import { useRouter } from 'next/router'

export default function Page(props) {
const router = useRouter()

return (
<>
<p id="page">/blog/[slug]</p>
<p id="props">{JSON.stringify(props)}</p>
<p id="asPath">{router.asPath}</p>
<p id="pathname">{router.pathname}</p>
<p id="query">{JSON.stringify(router.query)}</p>
</>
)
}

export function getStaticProps({ params }) {
console.log('getStaticProps /blog/[slug]', params)
return {
props: {
params,
now: Date.now(),
},
}
}

export function getStaticPaths() {
return {
paths: ['/blog/first'],
fallback: 'blocking',
}
}
25 changes: 25 additions & 0 deletions test/production/minimal-mode-response-cache/app/pages/index.js
@@ -0,0 +1,25 @@
import { useRouter } from 'next/router'

export default function Page(props) {
const router = useRouter()

return (
<>
<p id="page">/</p>
<p id="props">{JSON.stringify(props)}</p>
<p id="asPath">{router.asPath}</p>
<p id="pathname">{router.pathname}</p>
<p id="query">{JSON.stringify(router.query)}</p>
</>
)
}

export function getStaticProps() {
console.log('getStaticProps /')
return {
props: {
index: true,
now: Date.now(),
},
}
}
25 changes: 25 additions & 0 deletions test/production/minimal-mode-response-cache/app/pages/news.js
@@ -0,0 +1,25 @@
import { useRouter } from 'next/router'

export default function Page(props) {
const router = useRouter()

return (
<>
<p id="page">/news</p>
<p id="props">{JSON.stringify(props)}</p>
<p id="asPath">{router.asPath}</p>
<p id="pathname">{router.pathname}</p>
<p id="query">{JSON.stringify(router.query)}</p>
</>
)
}

export function getStaticProps() {
console.log('getStaticProps /news')
return {
props: {
news: true,
now: Date.now(),
},
}
}
122 changes: 122 additions & 0 deletions test/production/minimal-mode-response-cache/index.test.ts
@@ -0,0 +1,122 @@
import glob from 'glob'
import fs from 'fs-extra'
import { join } from 'path'
import cheerio from 'cheerio'
import { createNext, FileRef } from 'e2e-utils'
import { NextInstance } from 'test/lib/next-modes/base'
import {
killApp,
findPort,
renderViaHTTP,
initNextServerScript,
} from 'next-test-utils'

describe('minimal-mode-response-cache', () => {
let next: NextInstance
let server
let appPort

beforeAll(async () => {
// test build against environment with next support
process.env.NOW_BUILDER = '1'

next = await createNext({
files: new FileRef(join(__dirname, 'app')),
})
await next.stop()

await fs.move(
join(next.testDir, '.next/standalone'),
join(next.testDir, 'standalone')
)
for (const file of await fs.readdir(next.testDir)) {
if (file !== 'standalone') {
await fs.remove(join(next.testDir, file))
console.log('removed', file)
}
}
const files = glob.sync('**/*', {
cwd: join(next.testDir, 'standalone/.next/server/pages'),
dot: true,
})

for (const file of files) {
if (file.endsWith('.json') || file.endsWith('.html')) {
await fs.remove(join(next.testDir, '.next/server', file))
}
}

const testServer = join(next.testDir, 'standalone/server.js')
await fs.writeFile(
testServer,
(await fs.readFile(testServer, 'utf8'))
.replace('console.error(err)', `console.error('top-level', err)`)
.replace('conf:', 'minimalMode: true,conf:')
)
appPort = await findPort()
server = await initNextServerScript(
testServer,
/Listening on/,
{
...process.env,
PORT: appPort,
},
undefined,
{
cwd: next.testDir,
}
)
})
afterAll(async () => {
await next.destroy()
if (server) await killApp(server)
})

it('should have correct responses', async () => {
const html = await renderViaHTTP(appPort, '/')
expect(html.length).toBeTruthy()

for (const { path, matchedPath, query, asPath, pathname } of [
Copy link
Member

Choose a reason for hiding this comment

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

Nit pick: You can use jest .each() which will print much easier to read error message when the assertions fail.

Example usage:

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 timing here is important as we also want to ensure a previous cache entry isn't re-used within the 1 second window so didn't break it into separate it() checks as that could add some overhead

{ path: '/', asPath: '/' },
{ path: '/', matchedPath: '/index', asPath: '/' },
{ path: '/', matchedPath: '/index/', asPath: '/' },
{ path: '/', matchedPath: '/', asPath: '/' },
{
path: '/news/',
matchedPath: '/news/',
asPath: '/news/',
pathname: '/news',
},
{
path: '/news/',
matchedPath: '/news',
asPath: '/news/',
pathname: '/news',
},
{
path: '/blog/first/',
matchedPath: '/blog/first/',
pathname: '/blog/[slug]',
asPath: '/blog/first/',
query: { slug: 'first' },
},
{
path: '/blog/second/',
matchedPath: '/blog/[slug]/',
pathname: '/blog/[slug]',
asPath: '/blog/second/',
query: { slug: 'second' },
},
]) {
const html = await renderViaHTTP(appPort, path, undefined, {
headers: {
'x-matched-path': matchedPath || path,
},
})
const $ = cheerio.load(html)
expect($('#asPath').text()).toBe(asPath)
expect($('#pathname').text()).toBe(pathname || path)
expect(JSON.parse($('#query').text())).toEqual(query || {})
}
})
})