From 89ca0d10d45b1d8fce6cb0358a2662c27b61a850 Mon Sep 17 00:00:00 2001 From: JJ Kasper Date: Mon, 29 Jun 2020 10:14:45 -0500 Subject: [PATCH] Update to use getDataHref in fetchNextData (#14667) This updates `fetchNextData` to re-use the `getDataHref` function from `page-loader` which has more verbose handling to ensure the correct `/_next/data` URL is built. Re-using this logic ensures the `/_next/data` URL can still be built even when a mismatching `href` and `as` value is provided to `next/link`. This also fixes a case in `getDataHref` where optional values that weren't provided would fail to build the data href since the check requiring the param be present while interpolating the route values hasn't been updated to allow missing params for optional values. An additional test case has been added to the prerender suite to ensure the `/_next/data` URL is built correctly when mismatching `href` and `as` values are provided x-ref: https://github.com/vercel/next.js/discussions/14536 x-ref: https://github.com/vercel/next.js/discussions/9081#discussioncomment-31160 Closes: https://github.com/vercel/next.js/issues/14668 --- packages/next/client/page-loader.js | 25 ++++--- .../next/next-server/lib/router/router.ts | 73 +++++++++---------- .../build-output/test/index.test.js | 2 +- test/integration/prerender/pages/index.js | 4 + .../prerender/pages/lang/[lang]/about.js | 2 +- test/integration/prerender/test/index.test.js | 40 ++++++++++ 6 files changed, 92 insertions(+), 54 deletions(-) diff --git a/packages/next/client/page-loader.js b/packages/next/client/page-loader.js index c2960466e5..9f54bd70ea 100644 --- a/packages/next/client/page-loader.js +++ b/packages/next/client/page-loader.js @@ -109,17 +109,18 @@ export default class PageLoader { * @param {string} href the route href (file-system path) * @param {string} asPath the URL as shown in browser (virtual path); used for dynamic routes */ - getDataHref(href, asPath) { + getDataHref(href, asPath, ssg) { + const { pathname: hrefPathname, query, search } = parse(href, true) + const { pathname: asPathname } = parse(asPath) + const route = normalizeRoute(hrefPathname) + const getHrefForSlug = (/** @type string */ path) => { const dataRoute = getAssetPathFromRoute(path, '.json') - return `${this.assetPrefix}/_next/data/${this.buildId}${dataRoute}` + return `${this.assetPrefix}/_next/data/${this.buildId}${dataRoute}${ + ssg ? '' : search || '' + }` } - const { pathname: hrefPathname, query } = parse(href, true) - const { pathname: asPathname } = parse(asPath) - - const route = normalizeRoute(hrefPathname) - let isDynamic = isDynamicRoute(route), interpolatedRoute if (isDynamic) { @@ -135,19 +136,19 @@ export default class PageLoader { interpolatedRoute = route if ( !Object.keys(dynamicGroups).every((param) => { - let value = dynamicMatches[param] + let value = dynamicMatches[param] || '' const { repeat, optional } = dynamicGroups[param] // support single-level catch-all // TODO: more robust handling for user-error (passing `/`) - if (repeat && !Array.isArray(value)) value = [value] let replaced = `[${repeat ? '...' : ''}${param}]` if (optional) { - replaced = `[${replaced}]` + replaced = `${!value ? '/' : ''}[${replaced}]` } + if (repeat && !Array.isArray(value)) value = [value] return ( - param in dynamicMatches && + (optional || param in dynamicMatches) && // Interpolate group into data URL if present (interpolatedRoute = interpolatedRoute.replace( replaced, @@ -182,7 +183,7 @@ export default class PageLoader { // Check if the route requires a data file s.has(route) && // Try to generate data href, noop when falsy - (_dataHref = this.getDataHref(href, asPath)) && + (_dataHref = this.getDataHref(href, asPath, true)) && // noop when data has already been prefetched (dedupe) !document.querySelector( `link[rel="${relPrefetch}"][href^="${_dataHref}"]` diff --git a/packages/next/next-server/lib/router/router.ts b/packages/next/next-server/lib/router/router.ts index 2acfa7fcd0..e92e603d44 100644 --- a/packages/next/next-server/lib/router/router.ts +++ b/packages/next/next-server/lib/router/router.ts @@ -16,7 +16,6 @@ import { isDynamicRoute } from './utils/is-dynamic' import { getRouteMatcher } from './utils/route-matcher' import { getRouteRegex } from './utils/route-regex' import { normalizeTrailingSlash } from './normalize-trailing-slash' -import getAssetPathFromRoute from './utils/get-asset-path-from-route' const basePath = (process.env.__NEXT_ROUTER_BASEPATH as string) || '' @@ -108,39 +107,26 @@ type ComponentLoadCancel = (() => void) | null type HistoryMethod = 'replaceState' | 'pushState' function fetchNextData( - pathname: string, - query: ParsedUrlQuery | null, + dataHref: string, isServerRender: boolean, cb?: (...args: any) => any ) { let attempts = isServerRender ? 3 : 1 function getResponse(): Promise { - return fetch( - formatWithValidation({ - pathname: addBasePath( - // @ts-ignore __NEXT_DATA__ - `/_next/data/${__NEXT_DATA__.buildId}${getAssetPathFromRoute( - pathname, - '.json' - )}` - ), - query, - }), - { - // Cookies are required to be present for Next.js' SSG "Preview Mode". - // Cookies may also be required for `getServerSideProps`. - // - // > `fetch` won’t send cookies, unless you set the credentials init - // > option. - // https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API/Using_Fetch - // - // > For maximum browser compatibility when it comes to sending & - // > receiving cookies, always supply the `credentials: 'same-origin'` - // > option instead of relying on the default. - // https://github.com/github/fetch#caveats - credentials: 'same-origin', - } - ).then((res) => { + return fetch(dataHref, { + // Cookies are required to be present for Next.js' SSG "Preview Mode". + // Cookies may also be required for `getServerSideProps`. + // + // > `fetch` won’t send cookies, unless you set the credentials init + // > option. + // https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API/Using_Fetch + // + // > For maximum browser compatibility when it comes to sending & + // > receiving cookies, always supply the `credentials: 'same-origin'` + // > option instead of relying on the default. + // https://github.com/github/fetch#caveats + credentials: 'same-origin', + }).then((res) => { if (!res.ok) { if (--attempts > 0 && res.status >= 500) { return getResponse() @@ -669,11 +655,21 @@ export default class Router implements BaseRouter { } } + let dataHref: string | undefined + + if (__N_SSG || __N_SSP) { + dataHref = this.pageLoader.getDataHref( + formatWithValidation({ pathname, query }), + as, + __N_SSG + ) + } + return this._getData(() => __N_SSG - ? this._getStaticData(as) + ? this._getStaticData(dataHref!) : __N_SSP - ? this._getServerData(as) + ? this._getServerData(dataHref!) : this.getInitialProps( Component, // we provide AppTree later so this needs to be `any` @@ -843,23 +839,20 @@ export default class Router implements BaseRouter { }) } - _getStaticData = (asPath: string): Promise => { - const pathname = prepareRoute(parse(asPath).pathname!) + _getStaticData = (dataHref: string): Promise => { + const pathname = prepareRoute(parse(dataHref).pathname!) return process.env.NODE_ENV === 'production' && this.sdc[pathname] - ? Promise.resolve(this.sdc[pathname]) + ? Promise.resolve(this.sdc[dataHref]) : fetchNextData( - pathname, - null, + dataHref, this.isSsr, (data) => (this.sdc[pathname] = data) ) } - _getServerData = (asPath: string): Promise => { - let { pathname, query } = parse(asPath, true) - pathname = prepareRoute(pathname!) - return fetchNextData(pathname, query, this.isSsr) + _getServerData = (dataHref: string): Promise => { + return fetchNextData(dataHref, this.isSsr) } getInitialProps( diff --git a/test/integration/build-output/test/index.test.js b/test/integration/build-output/test/index.test.js index 8064d3aa32..efa0151d49 100644 --- a/test/integration/build-output/test/index.test.js +++ b/test/integration/build-output/test/index.test.js @@ -113,7 +113,7 @@ describe('Build Output', () => { expect(parseFloat(webpackSize) - 775).toBeLessThanOrEqual(0) expect(webpackSize.endsWith('B')).toBe(true) - expect(parseFloat(mainSize) - 6.3).toBeLessThanOrEqual(0) + expect(parseFloat(mainSize) - 6.4).toBeLessThanOrEqual(0) expect(mainSize.endsWith('kB')).toBe(true) expect(parseFloat(frameworkSize) - 41).toBeLessThanOrEqual(0) diff --git a/test/integration/prerender/pages/index.js b/test/integration/prerender/pages/index.js index 240a501bef..ac34b844f7 100644 --- a/test/integration/prerender/pages/index.js +++ b/test/integration/prerender/pages/index.js @@ -52,6 +52,10 @@ const Page = ({ world, time }) => { to nested index
+ + to rewritten static path page + +
to optional catchall root diff --git a/test/integration/prerender/pages/lang/[lang]/about.js b/test/integration/prerender/pages/lang/[lang]/about.js index 693a814883..a985dea17b 100644 --- a/test/integration/prerender/pages/lang/[lang]/about.js +++ b/test/integration/prerender/pages/lang/[lang]/about.js @@ -1,4 +1,4 @@ -export default ({ lang }) =>

About: {lang}

+export default ({ lang }) =>

About: {lang}

export const getStaticProps = ({ params: { lang } }) => ({ props: { diff --git a/test/integration/prerender/test/index.test.js b/test/integration/prerender/test/index.test.js index 52c36ce685..23f5c82831 100644 --- a/test/integration/prerender/test/index.test.js +++ b/test/integration/prerender/test/index.test.js @@ -25,6 +25,7 @@ import { } from 'next-test-utils' import webdriver from 'next-webdriver' import { dirname, join } from 'path' +import url from 'url' jest.setTimeout(1000 * 60 * 2) const appDir = join(__dirname, '..') @@ -601,6 +602,45 @@ const runTests = (dev = false, isEmulatedServerless = false) => { const html = await renderViaHTTP(appPort, '/about') expect(html).toMatch(/About:.*?en/) }) + + it('should fetch /_next/data correctly with mismatched href and as', async () => { + const browser = await webdriver(appPort, '/') + + if (!dev) { + await browser.eval(() => + document.querySelector('#to-rewritten-ssg').scrollIntoView() + ) + + await check( + async () => { + const links = await browser.elementsByCss('link[rel=prefetch]') + let found = false + + for (const link of links) { + const href = await link.getAttribute('href') + const { pathname } = url.parse(href) + + if (pathname.endsWith('/lang/en/about.json')) { + found = true + break + } + } + return found + }, + { + test(result) { + return result === true + }, + } + ) + } + await browser.eval('window.beforeNav = "hi"') + await browser.elementByCss('#to-rewritten-ssg').click() + await browser.waitForElementByCss('#about') + + expect(await browser.eval('window.beforeNav')).toBe('hi') + expect(await browser.elementByCss('#about').text()).toBe('About: en') + }) } if (dev) { -- GitLab