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

Fix failing E2E deployment test cases #36368

Merged
merged 2 commits into from Apr 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 @@ -30,7 +30,8 @@ export function getApiHandler(ctx: ServerlessHandlerCtx) {
// We need to trust the dynamic route params from the proxy
// to ensure we are using the correct values
const trustQuery = req.headers[vercelHeader]
const parsedUrl = handleRewrites(req, parseUrl(req.url!, true))
const parsedUrl = parseUrl(req.url!, true)
handleRewrites(req, parsedUrl)

if (parsedUrl.query.nextInternalLocale) {
delete parsedUrl.query.nextInternalLocale
Expand Down
Expand Up @@ -139,7 +139,7 @@ export function getPageHandler(ctx: ServerlessHandlerCtx) {
}
const origQuery = Object.assign({}, parsedUrl.query)

parsedUrl = handleRewrites(req, parsedUrl)
handleRewrites(req, parsedUrl)
handleBasePath(req, parsedUrl)

// remove ?amp=1 from request URL if rendering for export
Expand Down
Expand Up @@ -91,6 +91,8 @@ export function getUtils({
req: BaseNextRequest | IncomingMessage,
parsedUrl: UrlWithParsedQuery
) {
const rewriteParams = {}

for (const rewrite of rewrites) {
const matcher = getCustomRouteMatcher(rewrite.source)
let params = matcher(parsedUrl.pathname)
Expand All @@ -106,13 +108,14 @@ export function getUtils({
}

if (params) {
const { parsedDestination } = prepareDestination({
const { parsedDestination, destQuery } = prepareDestination({
appendParamsToQuery: true,
destination: rewrite.destination,
params: params,
query: parsedUrl.query,
})

Object.assign(rewriteParams, destQuery, params)
Object.assign(parsedUrl.query, parsedDestination.query)
delete (parsedDestination as any).query

Expand Down Expand Up @@ -152,7 +155,7 @@ export function getUtils({
}
}

return parsedUrl
return rewriteParams
}

function handleBasePath(
Expand Down Expand Up @@ -285,15 +288,16 @@ export function getUtils({

function normalizeVercelUrl(
req: BaseNextRequest | IncomingMessage,
trustQuery: boolean
trustQuery: boolean,
paramKeys?: string[]
) {
// make sure to normalize req.url on Vercel to strip dynamic params
// from the query which are added during routing
if (pageIsDynamic && trustQuery && defaultRouteRegex) {
const _parsedUrl = parseUrl(req.url!, true)
delete (_parsedUrl as any).search

for (const param of Object.keys(defaultRouteRegex.groups)) {
for (const param of paramKeys || Object.keys(defaultRouteRegex.groups)) {
delete _parsedUrl.query[param]
}
req.url = formatUrl(_parsedUrl)
Expand Down
41 changes: 34 additions & 7 deletions packages/next/server/base-server.ts
Expand Up @@ -440,21 +440,26 @@ export default abstract class Server {
typeof req.headers['x-matched-path'] === 'string'
) {
const reqUrlIsDataUrl = req.url?.includes('/_next/data')
const parsedMatchedPath = parseUrl(req.headers['x-matched-path'] || '')
const matchedPathIsDataUrl =
req.headers['x-matched-path']?.includes('/_next/data')
parsedMatchedPath.pathname?.includes('/_next/data')
const isDataUrl = reqUrlIsDataUrl || matchedPathIsDataUrl

let parsedPath = parseUrl(
isDataUrl ? req.url! : (req.headers['x-matched-path'] as string),
true
)

let matchedPathname = parsedPath.pathname!

let matchedPathnameNoExt = isDataUrl
? matchedPathname.replace(/\.json$/, '')
: matchedPathname

let srcPathname = isDataUrl
? parsedMatchedPath.pathname?.replace(/\.json$/, '') ||
matchedPathnameNoExt
: matchedPathnameNoExt

if (this.nextConfig.i18n) {
const localePathResult = normalizeLocalePath(
matchedPathname || '/',
Expand All @@ -469,9 +474,10 @@ export default abstract class Server {
if (isDataUrl) {
matchedPathname = denormalizePagePath(matchedPathname)
matchedPathnameNoExt = denormalizePagePath(matchedPathnameNoExt)
srcPathname = denormalizePagePath(srcPathname)
}

const pageIsDynamic = isDynamicRoute(matchedPathnameNoExt)
const pageIsDynamic = isDynamicRoute(srcPathname)
const combinedRewrites: Rewrite[] = []

combinedRewrites.push(...this.customRoutes.rewrites.beforeFiles)
Expand All @@ -480,7 +486,7 @@ export default abstract class Server {

const utils = getUtils({
pageIsDynamic,
page: matchedPathnameNoExt,
page: srcPathname,
i18n: this.nextConfig.i18n,
basePath: this.nextConfig.basePath,
rewrites: combinedRewrites,
Expand All @@ -492,7 +498,15 @@ export default abstract class Server {
if (this.nextConfig.i18n && !url.locale?.path.detectedLocale) {
parsedUrl.pathname = `/${url.locale?.locale}${parsedUrl.pathname}`
}
utils.handleRewrites(req, parsedUrl)
const pathnameBeforeRewrite = parsedUrl.pathname
const rewriteParams = utils.handleRewrites(req, parsedUrl)
const rewriteParamKeys = Object.keys(rewriteParams)
const didRewrite = pathnameBeforeRewrite !== parsedUrl.pathname

if (didRewrite) {
addRequestMeta(req, '_nextRewroteUrl', parsedUrl.pathname!)
addRequestMeta(req, '_nextDidRewrite', true)
}

// interpolate dynamic params and normalize URL if needed
if (pageIsDynamic) {
Expand Down Expand Up @@ -538,9 +552,14 @@ export default abstract class Server {
pathname: matchedPathname,
})
}

Object.assign(parsedUrl.query, params)
utils.normalizeVercelUrl(req, true)
}

if (pageIsDynamic || didRewrite) {
utils.normalizeVercelUrl(req, true, [
...rewriteParamKeys,
...Object.keys(utils.defaultRouteRegex?.groups || {}),
])
}
} catch (err) {
if (err instanceof DecodeError) {
Expand Down Expand Up @@ -1368,6 +1387,14 @@ export default abstract class Server {
isRedirect = renderResult.renderOpts.isRedirect
} else {
const origQuery = parseUrl(req.url || '', true).query

// clear any dynamic route params so they aren't in
// the resolvedUrl
if (opts.params) {
Object.keys(opts.params).forEach((key) => {
delete origQuery[key]
})
}
const hadTrailingSlash =
urlPathname !== '/' && this.nextConfig.trailingSlash

Expand Down
Expand Up @@ -217,6 +217,7 @@ export function prepareDestination(args: {

return {
newUrl,
destQuery,
parsedDestination,
}
}
Expand Down
7 changes: 6 additions & 1 deletion test/e2e/prerender.test.ts
Expand Up @@ -844,7 +844,12 @@ describe('Prerender', () => {

it('should 404 for an invalid data url', async () => {
const res = await fetchViaHTTP(next.url, `/_next/data/${next.buildId}`)
expect(res.status).toBe(404)

// when deployed this will match due to `index.json` matching the
// directory itself
if (!isDeploy) {
expect(res.status).toBe(404)
}
})

it('should allow rewriting to SSG page with fallback: false', async () => {
Expand Down
42 changes: 42 additions & 0 deletions test/production/required-server-files.test.ts
Expand Up @@ -54,6 +54,10 @@ describe('should set-up next', () => {
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 @@ -667,6 +671,44 @@ describe('should set-up next', () => {
expect(await res.text()).toContain('Bad Request')
})

it('should have correct resolvedUrl from rewrite', async () => {
const res = await fetchViaHTTP(appPort, '/to-dynamic/post-1', undefined, {
headers: {
'x-matched-path': '/dynamic/[slug]',
},
})
expect(res.status).toBe(200)
const $ = cheerio.load(await res.text())
expect($('#resolved-url').text()).toBe('/dynamic/post-1')
})

it('should have correct resolvedUrl from rewrite with added query', async () => {
const res = await fetchViaHTTP(appPort, '/to-dynamic/post-2', undefined, {
headers: {
'x-matched-path': '/dynamic/[slug]',
},
})
expect(res.status).toBe(200)
const $ = cheerio.load(await res.text())
expect($('#resolved-url').text()).toBe('/dynamic/post-2')
})

it('should have correct resolvedUrl from dynamic route', async () => {
const res = await fetchViaHTTP(
appPort,
`/_next/data/${next.buildId}/dynamic/post-2.json`,
{ slug: 'post-2' },
{
headers: {
'x-matched-path': '/dynamic/[slug]',
},
}
)
expect(res.status).toBe(200)
const json = await res.json()
expect(json.pageProps.resolvedUrl).toBe('/dynamic/post-2')
})

it('should bubble error correctly for gip page', async () => {
errors = []
const res = await fetchViaHTTP(appPort, '/errors/gip', { crash: '1' })
Expand Down
@@ -1,8 +1,9 @@
import { useRouter } from 'next/router'

export const getServerSideProps = ({ params }) => {
export const getServerSideProps = ({ params, resolvedUrl }) => {
return {
props: {
resolvedUrl,
hello: 'world',
slug: params.slug,
random: Math.random(),
Expand All @@ -16,6 +17,7 @@ export default function Page(props) {
<>
<p id="dynamic">dynamic page</p>
<p id="slug">{props.slug}</p>
<p id="resolved-url">{props.resolvedUrl}</p>
<p id="router">{JSON.stringify(router)}</p>
<p id="props">{JSON.stringify(props)}</p>
</>
Expand Down