From d3f4a4cb2afc9e18a82609d907d50a311578470f Mon Sep 17 00:00:00 2001 From: JJ Kasper Date: Mon, 14 Sep 2020 16:01:04 -0500 Subject: [PATCH] Provide resolvedUrl to getServerSideProps (#17082) This continues off of https://github.com/vercel/next.js/pull/17081 and provides this normalized `asPath` value in the context provided to `getServerSideProps` to provide the consistent value since the request URL can vary between direct visit and client transition and the alternative requires building the URL each time manually. Kept this change separate from https://github.com/vercel/next.js/pull/17081 since this is addressing a separate issue and allows discussion separately. Closes: https://github.com/vercel/next.js/issues/16407 --- docs/basic-features/data-fetching.md | 1 + .../webpack/loaders/next-serverless-loader.ts | 8 +- .../next/next-server/server/next-server.ts | 12 +-- packages/next/next-server/server/render.tsx | 6 +- packages/next/types/index.d.ts | 1 + .../getserversideprops/next.config.js | 19 ++++ .../pages/blog/[post]/index.js | 6 +- .../getserversideprops/pages/something.js | 14 ++- .../getserversideprops/test/index.test.js | 100 ++++++++++++++++-- 9 files changed, 141 insertions(+), 26 deletions(-) create mode 100644 test/integration/getserversideprops/next.config.js diff --git a/docs/basic-features/data-fetching.md b/docs/basic-features/data-fetching.md index b16d843eb5..905385db53 100644 --- a/docs/basic-features/data-fetching.md +++ b/docs/basic-features/data-fetching.md @@ -543,6 +543,7 @@ The `context` parameter is an object containing the following keys: - `query`: The query string. - `preview`: `preview` is `true` if the page is in the preview mode and `false` otherwise. See the [Preview Mode documentation](/docs/advanced-features/preview-mode.md). - `previewData`: The preview data set by `setPreviewData`. See the [Preview Mode documentation](/docs/advanced-features/preview-mode.md). +- `resolvedUrl`: A normalized version of the request URL that strips the `_next/data` prefix for client transitions and includes original query values. > **Note**: You can import modules in top-level scope for use in `getServerSideProps`. > Imports used in `getServerSideProps` will not be bundled for the client-side. diff --git a/packages/next/build/webpack/loaders/next-serverless-loader.ts b/packages/next/build/webpack/loaders/next-serverless-loader.ts index 62e26fe066..4f6d882aa7 100644 --- a/packages/next/build/webpack/loaders/next-serverless-loader.ts +++ b/packages/next/build/webpack/loaders/next-serverless-loader.ts @@ -496,7 +496,7 @@ const nextServerlessLoader: loader.Loader = function () { !fromExport && (getStaticProps || getServerSideProps) ) { - const curQuery = {...parsedUrl.query} + const origQuery = parseUrl(req.url, true).query ${ pageIsDynamicRoute @@ -507,7 +507,7 @@ const nextServerlessLoader: loader.Loader = function () { delete parsedUrl.search for (const param of Object.keys(defaultRouteRegex.groups)) { - delete curQuery[param] + delete origQuery[param] } } ` @@ -515,9 +515,9 @@ const nextServerlessLoader: loader.Loader = function () { } parsedUrl.pathname = denormalizePagePath(parsedUrl.pathname) - renderOpts.normalizedAsPath = formatUrl({ + renderOpts.resolvedUrl = formatUrl({ ...parsedUrl, - query: curQuery + query: origQuery }) } diff --git a/packages/next/next-server/server/next-server.ts b/packages/next/next-server/server/next-server.ts index a280a5423b..6b91b92037 100644 --- a/packages/next/next-server/server/next-server.ts +++ b/packages/next/next-server/server/next-server.ts @@ -1111,13 +1111,11 @@ export default class Server { ...components, ...opts, isDataReq, - normalizedAsPath: isDataReq - ? formatUrl({ - pathname: urlPathname, - // make sure to only add query values from original URL - query: parseUrl(req.url!, true).query, - }) - : undefined, + resolvedUrl: formatUrl({ + pathname: urlPathname, + // make sure to only add query values from original URL + query: parseUrl(req.url || '', true).query, + }), } renderResult = await renderToHTML( diff --git a/packages/next/next-server/server/render.tsx b/packages/next/next-server/server/render.tsx index f3fc22eede..1ecb16b212 100644 --- a/packages/next/next-server/server/render.tsx +++ b/packages/next/next-server/server/render.tsx @@ -154,7 +154,7 @@ export type RenderOptsPartial = { fontManifest?: FontManifest optimizeImages: boolean devOnlyCacheBusterQueryString?: string - normalizedAsPath?: string + resolvedUrl?: string } export type RenderOpts = LoadComponentsReturnType & RenderOptsPartial @@ -476,6 +476,7 @@ export async function renderToHTML( : {}), } req.url = pathname + renderOpts.resolvedUrl = pathname renderOpts.nextExport = true } @@ -489,7 +490,7 @@ export async function renderToHTML( await Loadable.preloadAll() // Make sure all dynamic imports are loaded // url will always be set - const asPath: string = renderOpts.normalizedAsPath || (req.url as string) + const asPath: string = renderOpts.resolvedUrl || (req.url as string) const router = new ServerRouter( pathname, query, @@ -689,6 +690,7 @@ export async function renderToHTML( req, res, query, + resolvedUrl: asPath, ...(pageIsDynamic ? { params: params as ParsedUrlQuery } : undefined), ...(previewData !== false ? { preview: true, previewData: previewData } diff --git a/packages/next/types/index.d.ts b/packages/next/types/index.d.ts index 1c047e889c..7416c841dd 100644 --- a/packages/next/types/index.d.ts +++ b/packages/next/types/index.d.ts @@ -120,6 +120,7 @@ export type GetServerSidePropsContext< query: ParsedUrlQuery preview?: boolean previewData?: any + resolvedUrl: string } export type GetServerSidePropsResult

= { diff --git a/test/integration/getserversideprops/next.config.js b/test/integration/getserversideprops/next.config.js new file mode 100644 index 0000000000..ac7d2bcaf0 --- /dev/null +++ b/test/integration/getserversideprops/next.config.js @@ -0,0 +1,19 @@ +module.exports = { + // replace me + async rewrites() { + return [ + { + source: '/blog-post-1', + destination: '/blog/post-1', + }, + { + source: '/blog-post-2', + destination: '/blog/post-2?hello=world', + }, + { + source: '/blog-:param', + destination: '/blog/post-3', + }, + ] + }, +} diff --git a/test/integration/getserversideprops/pages/blog/[post]/index.js b/test/integration/getserversideprops/pages/blog/[post]/index.js index 354d3adef0..799cfb0664 100644 --- a/test/integration/getserversideprops/pages/blog/[post]/index.js +++ b/test/integration/getserversideprops/pages/blog/[post]/index.js @@ -2,7 +2,7 @@ import React from 'react' import Link from 'next/link' import { useRouter } from 'next/router' -export async function getServerSideProps({ params }) { +export async function getServerSideProps({ params, resolvedUrl }) { if (params.post === 'post-10') { await new Promise((resolve) => { setTimeout(() => resolve(), 1000) @@ -16,13 +16,14 @@ export async function getServerSideProps({ params }) { return { props: { params, + resolvedUrl, post: params.post, time: (await import('perf_hooks')).performance.now(), }, } } -export default ({ post, time, params, appProps }) => { +export default ({ post, time, params, appProps, resolvedUrl }) => { return ( <>

Post: {post}

@@ -31,6 +32,7 @@ export default ({ post, time, params, appProps }) => {
{JSON.stringify(useRouter().query)}
{JSON.stringify(appProps.query)}
{appProps.url}
+
{resolvedUrl}
to home diff --git a/test/integration/getserversideprops/pages/something.js b/test/integration/getserversideprops/pages/something.js index 73b8374428..fc209e8746 100644 --- a/test/integration/getserversideprops/pages/something.js +++ b/test/integration/getserversideprops/pages/something.js @@ -2,9 +2,10 @@ import React from 'react' import Link from 'next/link' import { useRouter } from 'next/router' -export async function getServerSideProps({ params, query }) { +export async function getServerSideProps({ params, query, resolvedUrl }) { return { props: { + resolvedUrl: resolvedUrl, world: 'world', query: query || {}, params: params || {}, @@ -14,7 +15,15 @@ export async function getServerSideProps({ params, query }) { } } -export default ({ world, time, params, random, query, appProps }) => { +export default ({ + world, + time, + params, + random, + query, + appProps, + resolvedUrl, +}) => { return ( <>

hello: {world}

@@ -25,6 +34,7 @@ export default ({ world, time, params, random, query, appProps }) => {
{JSON.stringify(useRouter().query)}
{JSON.stringify(appProps.query)}
{appProps.url}
+
{resolvedUrl}
to home diff --git a/test/integration/getserversideprops/test/index.test.js b/test/integration/getserversideprops/test/index.test.js index 1347346d4c..b675ff64dc 100644 --- a/test/integration/getserversideprops/test/index.test.js +++ b/test/integration/getserversideprops/test/index.test.js @@ -7,6 +7,7 @@ import { check, fetchViaHTTP, findPort, + File, getBrowserBodyText, getRedboxHeader, killApp, @@ -22,7 +23,8 @@ import { join } from 'path' jest.setTimeout(1000 * 60 * 2) const appDir = join(__dirname, '..') -const nextConfig = join(appDir, 'next.config.js') +const nextConfig = new File(join(appDir, 'next.config.js')) + let app let appPort let buildId @@ -295,7 +297,9 @@ const runTests = (dev = false) => { it('should have original req.url for /_next/data request dynamic page', async () => { const curUrl = `/_next/data/${buildId}/blog/post-1.json` const data = await renderViaHTTP(appPort, curUrl) - const { appProps } = JSON.parse(data) + const { appProps, pageProps } = JSON.parse(data) + + expect(pageProps.resolvedUrl).toEqual('/blog/post-1') expect(appProps).toEqual({ url: curUrl, @@ -305,10 +309,27 @@ const runTests = (dev = false) => { }) }) + it('should have original req.url for /_next/data request dynamic page with query', async () => { + const curUrl = `/_next/data/${buildId}/blog/post-1.json` + const data = await renderViaHTTP(appPort, curUrl, { hello: 'world' }) + const { appProps, pageProps } = JSON.parse(data) + + expect(pageProps.resolvedUrl).toEqual('/blog/post-1?hello=world') + + expect(appProps).toEqual({ + url: curUrl + '?hello=world', + query: { post: 'post-1', hello: 'world' }, + asPath: '/blog/post-1?hello=world', + pathname: '/blog/[post]', + }) + }) + it('should have original req.url for /_next/data request', async () => { const curUrl = `/_next/data/${buildId}/something.json` const data = await renderViaHTTP(appPort, curUrl) - const { appProps } = JSON.parse(data) + const { appProps, pageProps } = JSON.parse(data) + + expect(pageProps.resolvedUrl).toEqual('/something') expect(appProps).toEqual({ url: curUrl, @@ -318,11 +339,70 @@ const runTests = (dev = false) => { }) }) + it('should have original req.url for /_next/data request with query', async () => { + const curUrl = `/_next/data/${buildId}/something.json` + const data = await renderViaHTTP(appPort, curUrl, { hello: 'world' }) + const { appProps, pageProps } = JSON.parse(data) + + expect(pageProps.resolvedUrl).toEqual('/something?hello=world') + + expect(appProps).toEqual({ + url: curUrl + '?hello=world', + query: { hello: 'world' }, + asPath: '/something?hello=world', + pathname: '/something', + }) + }) + it('should have correct req.url and query for direct visit dynamic page', async () => { const html = await renderViaHTTP(appPort, '/blog/post-1') const $ = cheerio.load(html) expect($('#app-url').text()).toContain('/blog/post-1') expect(JSON.parse($('#app-query').text())).toEqual({ post: 'post-1' }) + expect($('#resolved-url').text()).toBe('/blog/post-1') + }) + + it('should have correct req.url and query for direct visit dynamic page rewrite direct', async () => { + const html = await renderViaHTTP(appPort, '/blog-post-1') + const $ = cheerio.load(html) + expect($('#app-url').text()).toContain('/blog-post-1') + expect(JSON.parse($('#app-query').text())).toEqual({ post: 'post-1' }) + expect($('#resolved-url').text()).toBe('/blog/post-1') + }) + + it('should have correct req.url and query for direct visit dynamic page rewrite direct with internal query', async () => { + const html = await renderViaHTTP(appPort, '/blog-post-2') + const $ = cheerio.load(html) + expect($('#app-url').text()).toContain('/blog-post-2') + expect(JSON.parse($('#app-query').text())).toEqual({ + post: 'post-2', + hello: 'world', + }) + expect($('#resolved-url').text()).toBe('/blog/post-2') + }) + + it('should have correct req.url and query for direct visit dynamic page rewrite param', async () => { + const html = await renderViaHTTP(appPort, '/blog-post-3') + const $ = cheerio.load(html) + expect($('#app-url').text()).toContain('/blog-post-3') + expect(JSON.parse($('#app-query').text())).toEqual({ + post: 'post-3', + param: 'post-3', + }) + expect($('#resolved-url').text()).toBe('/blog/post-3') + }) + + it('should have correct req.url and query for direct visit dynamic page with query', async () => { + const html = await renderViaHTTP(appPort, '/blog/post-1', { + hello: 'world', + }) + const $ = cheerio.load(html) + expect($('#app-url').text()).toContain('/blog/post-1?hello=world') + expect(JSON.parse($('#app-query').text())).toEqual({ + post: 'post-1', + hello: 'world', + }) + expect($('#resolved-url').text()).toBe('/blog/post-1?hello=world') }) it('should have correct req.url and query for direct visit', async () => { @@ -330,6 +410,7 @@ const runTests = (dev = false) => { const $ = cheerio.load(html) expect($('#app-url').text()).toContain('/something') expect(JSON.parse($('#app-query').text())).toEqual({}) + expect($('#resolved-url').text()).toBe('/something') }) it('should return data correctly', async () => { @@ -587,10 +668,9 @@ describe('getServerSideProps', () => { describe('serverless mode', () => { beforeAll(async () => { - await fs.writeFile( - nextConfig, - `module.exports = { target: 'serverless' }`, - 'utf8' + await nextConfig.replace( + '// replace me', + `target: 'experimental-serverless-trace', ` ) await nextBuild(appDir) stderr = '' @@ -602,14 +682,16 @@ describe('getServerSideProps', () => { }) buildId = await fs.readFile(join(appDir, '.next/BUILD_ID'), 'utf8') }) - afterAll(() => killApp(app)) + afterAll(async () => { + await killApp(app) + nextConfig.restore() + }) runTests() }) describe('production mode', () => { beforeAll(async () => { - await fs.remove(nextConfig) await nextBuild(appDir, [], { stdout: true }) appPort = await findPort() -- GitLab