From edd798e52621f10402aa45d14660094dab0ee18a Mon Sep 17 00:00:00 2001 From: JJ Kasper Date: Fri, 24 Jun 2022 10:50:41 -0500 Subject: [PATCH] Fix hard navigation guard on popstate and handle fetching fresh data (#37970) * Fix hard navigation guard on popstate and handle fetching fresh data * undo link changes and handle middleware cases * update tests --- packages/next/cli/next-dev.ts | 4 +- packages/next/cli/next-start.ts | 4 +- packages/next/shared/lib/router/router.ts | 196 +++++++++++------- test/e2e/prerender.test.ts | 6 +- .../middleware-prefetch/tests/index.test.js | 1 - test/lib/browsers/base.ts | 3 + test/lib/next-modes/base.ts | 1 + test/lib/next-modes/next-dev.ts | 2 +- test/lib/next-modes/next-start.ts | 2 +- .../prerender-prefetch/index.test.ts | 107 +++++++++- 10 files changed, 235 insertions(+), 91 deletions(-) diff --git a/packages/next/cli/next-dev.ts b/packages/next/cli/next-dev.ts index b14355072e75b..5e0c22bb7f0bd 100755 --- a/packages/next/cli/next-dev.ts +++ b/packages/next/cli/next-dev.ts @@ -82,8 +82,8 @@ const nextDev: cliCommand = (argv) => { // we allow the server to use a random port while testing // instead of attempting to find a random port and then hope // it doesn't become occupied before we leverage it - if (process.env.__NEXT_RAND_PORT) { - port = 0 + if (process.env.__NEXT_FORCED_PORT) { + port = parseInt(process.env.__NEXT_FORCED_PORT, 10) || 0 } // We do not set a default host value here to prevent breaking diff --git a/packages/next/cli/next-start.ts b/packages/next/cli/next-start.ts index 09ad0f5b85732..b047cc27f6904 100755 --- a/packages/next/cli/next-start.ts +++ b/packages/next/cli/next-start.ts @@ -54,8 +54,8 @@ const nextStart: cliCommand = (argv) => { args['--port'] || (process.env.PORT && parseInt(process.env.PORT)) || 3000 const host = args['--hostname'] || '0.0.0.0' - if (process.env.__NEXT_RAND_PORT) { - port = 0 + if (process.env.__NEXT_FORCED_PORT) { + port = parseInt(process.env.__NEXT_FORCED_PORT, 10) || 0 } startServer({ diff --git a/packages/next/shared/lib/router/router.ts b/packages/next/shared/lib/router/router.ts index c7e876c994063..a738f700d44e8 100644 --- a/packages/next/shared/lib/router/router.ts +++ b/packages/next/shared/lib/router/router.ts @@ -62,6 +62,7 @@ interface TransitionOptions { shallow?: boolean locale?: string | false scroll?: boolean + unstable_skipClientCache?: boolean } interface NextHistoryState { @@ -339,6 +340,7 @@ export type NextRouter = BaseRouter & export type PrefetchOptions = { priority?: boolean locale?: string | false + unstable_skipClientCache?: boolean } export type PrivateRouteInfo = @@ -437,6 +439,7 @@ interface FetchNextDataParams { persistCache: boolean isPrefetch: boolean isBackground?: boolean + unstable_skipClientCache?: boolean } function fetchNextData({ @@ -448,6 +451,7 @@ function fetchNextData({ parseJSON, persistCache, isBackground, + unstable_skipClientCache, }: FetchNextDataParams): Promise { const { href: cacheKey } = new URL(dataHref, window.location.href) const getData = (params?: { method?: 'HEAD' | 'GET' }) => @@ -460,81 +464,90 @@ function fetchNextData({ return { dataHref, response, text: '', json: {} } } - return response - .text() - .then((text) => { - if (!response.ok) { - /** - * When the data response is a redirect because of a middleware - * we do not consider it an error. The headers must bring the - * mapped location. - * TODO: Change the status code in the handler. - */ - if ( - hasMiddleware && - [301, 302, 307, 308].includes(response.status) - ) { - return { dataHref, response, text, json: {} } - } - - if (response.status === 404) { - if (tryToParseAsJSON(text)?.notFound) { - return { - dataHref, - json: { notFound: SSG_DATA_NOT_FOUND }, - response, - text, - } - } + return response.text().then((text) => { + if (!response.ok) { + /** + * When the data response is a redirect because of a middleware + * we do not consider it an error. The headers must bring the + * mapped location. + * TODO: Change the status code in the handler. + */ + if ( + hasMiddleware && + [301, 302, 307, 308].includes(response.status) + ) { + return { dataHref, response, text, json: {} } + } - /** - * If there is a 404 that is not for SSG we used to fail but if - * there is a middleware we must respond with an empty object. - * For now we will return the data when there is a middleware. - * TODO: Update the server to success on these requests. - */ - if (hasMiddleware) { - return { dataHref, response, text, json: {} } + if (response.status === 404) { + if (tryToParseAsJSON(text)?.notFound) { + return { + dataHref, + json: { notFound: SSG_DATA_NOT_FOUND }, + response, + text, } } - const error = new Error(`Failed to load static props`) - /** - * We should only trigger a server-side transition if this was - * caused on a client-side transition. Otherwise, we'd get into - * an infinite loop. + * If there is a 404 that is not for SSG we used to fail but if + * there is a middleware we must respond with an empty object. + * For now we will return the data when there is a middleware. + * TODO: Update the server to success on these requests. */ - if (!isServerRender) { - markAssetError(error) + if (hasMiddleware) { + return { dataHref, response, text, json: {} } } - - throw error } - return { - dataHref, - json: parseJSON ? tryToParseAsJSON(text) : {}, - response, - text, - } - }) - .then((data) => { - if ( - !persistCache || - process.env.NODE_ENV !== 'production' || - data.response.headers.get('x-middleware-cache') === 'no-cache' - ) { - delete inflightCache[cacheKey] + const error = new Error(`Failed to load static props`) + + /** + * We should only trigger a server-side transition if this was + * caused on a client-side transition. Otherwise, we'd get into + * an infinite loop. + */ + if (!isServerRender) { + markAssetError(error) } - return data - }) + + throw error + } + + return { + dataHref, + json: parseJSON ? tryToParseAsJSON(text) : {}, + response, + text, + } + }) + }) + .then((data) => { + if ( + !persistCache || + process.env.NODE_ENV !== 'production' || + data.response.headers.get('x-middleware-cache') === 'no-cache' + ) { + delete inflightCache[cacheKey] + } + return data }) .catch((err) => { delete inflightCache[cacheKey] throw err }) + // when skipping client cache we wait to update + // inflight cache until successful data response + // this allows racing click event with fetching newer data + // without blocking navigation when stale data is available + if (unstable_skipClientCache && persistCache) { + return getData({}).then((data) => { + inflightCache[cacheKey] = Promise.resolve(data) + return data + }) + } + if (inflightCache[cacheKey] !== undefined) { return inflightCache[cacheKey] } @@ -559,16 +572,16 @@ export function createKey() { return Math.random().toString(36).slice(2, 10) } -function handleHardNavigation({ url }: { url: string }) { +function handleHardNavigation({ + url, + router, +}: { + url: string + router: Router +}) { // ensure we don't trigger a hard navigation to the same // URL as this can end up with an infinite refresh - const parsedUrl = new URL(url, location.href) - - if ( - parsedUrl.hostname === location.hostname && - parsedUrl.pathname === location.pathname && - parsedUrl.protocol === location.protocol - ) { + if (url === addBasePath(addLocale(router.asPath, router.locale))) { throw new Error( `Invariant: attempted to hard navigate to the same URL ${url} ${location.href}` ) @@ -949,7 +962,7 @@ export default class Router implements BaseRouter { forcedScroll?: { x: number; y: number } ): Promise { if (!isLocalURL(url)) { - handleHardNavigation({ url }) + handleHardNavigation({ url, router: this }) return false } // WARNING: `_h` is an internal option for handing Next.js client-side @@ -1020,7 +1033,10 @@ export default class Router implements BaseRouter { // if the locale isn't configured hard navigate to show 404 page if (!this.locales?.includes(nextState.locale!)) { parsedAs.pathname = addLocale(parsedAs.pathname, nextState.locale) - handleHardNavigation({ url: formatWithValidation(parsedAs) }) + handleHardNavigation({ + url: formatWithValidation(parsedAs), + router: this, + }) // this was previously a return but was removed in favor // of better dead code elimination with regenerator runtime didNavigate = true @@ -1055,6 +1071,7 @@ export default class Router implements BaseRouter { : `/${nextState.locale}` }${asNoBasePath === '/' ? '' : asNoBasePath}` || '/' )}`, + router: this, }) // this was previously a return but was removed in favor // of better dead code elimination with regenerator runtime @@ -1146,7 +1163,7 @@ export default class Router implements BaseRouter { } catch (err) { // If we fail to resolve the page list or client-build manifest, we must // do a server-side transition: - handleHardNavigation({ url: as }) + handleHardNavigation({ url: as, router: this }) return false } @@ -1194,7 +1211,7 @@ export default class Router implements BaseRouter { ) if (rewritesResult.externalDest) { - handleHardNavigation({ url: as }) + handleHardNavigation({ url: as, router: this }) return true } resolvedAs = rewritesResult.asPath @@ -1224,7 +1241,7 @@ export default class Router implements BaseRouter { `\nSee more info: https://nextjs.org/docs/messages/invalid-relative-url-external-as` ) } - handleHardNavigation({ url: as }) + handleHardNavigation({ url: as, router: this }) return false } @@ -1340,7 +1357,7 @@ export default class Router implements BaseRouter { if (routeInfo.type === 'redirect-internal') { return this.change(method, routeInfo.newUrl, routeInfo.newAs, options) } else { - handleHardNavigation({ url: routeInfo.destination }) + handleHardNavigation({ url: routeInfo.destination, router: this }) return new Promise(() => {}) } } @@ -1384,7 +1401,7 @@ export default class Router implements BaseRouter { ) return this.change(method, newUrl, newAs, options) } - handleHardNavigation({ url: destination }) + handleHardNavigation({ url: destination, router: this }) return new Promise(() => {}) } @@ -1548,7 +1565,10 @@ export default class Router implements BaseRouter { // 3. Internal error while loading the page // So, doing a hard reload is the proper way to deal with this. - handleHardNavigation({ url: as }) + handleHardNavigation({ + url: as, + router: this, + }) // Changing the URL doesn't block executing the current code path. // So let's throw a cancellation error stop the routing logic. @@ -1613,6 +1633,7 @@ export default class Router implements BaseRouter { locale, hasMiddleware, isPreview, + unstable_skipClientCache, }: { route: string pathname: string @@ -1623,6 +1644,7 @@ export default class Router implements BaseRouter { routeProps: RouteProperties locale: string | undefined isPreview: boolean + unstable_skipClientCache?: boolean }) { /** * This `route` binding can change if there's a rewrite @@ -1664,6 +1686,7 @@ export default class Router implements BaseRouter { inflightCache: this.sdc, persistCache: !isPreview, isPrefetch: false, + unstable_skipClientCache, } const data = await withMiddlewareEffects({ @@ -1758,6 +1781,7 @@ export default class Router implements BaseRouter { inflightCache: this.sdc, persistCache: !isPreview, isPrefetch: false, + unstable_skipClientCache, })) return { @@ -1997,6 +2021,10 @@ export default class Router implements BaseRouter { if (parsed.pathname !== pathname) { pathname = parsed.pathname parsed.pathname = pathname + Object.assign( + query, + getRouteMatcher(getRouteRegex(parsed.pathname))(asPath) || {} + ) url = formatWithValidation(parsed) } } @@ -2011,6 +2039,10 @@ export default class Router implements BaseRouter { ? options.locale || undefined : this.locale + // TODO: if the route middleware's data request + // resolves to is not an SSG route we should bust the cache + // but we shouldn't allow prefetch to keep triggering + // requests for SSP pages const data = await withMiddlewareEffects({ fetchData: () => fetchNextData({ @@ -2058,16 +2090,20 @@ export default class Router implements BaseRouter { this.pageLoader._isSsg(route).then((isSsg) => { return isSsg ? fetchNextData({ - dataHref: this.pageLoader.getDataHref({ - href: url, - asPath: resolvedAs, - locale: locale, - }), + dataHref: + data?.dataHref || + this.pageLoader.getDataHref({ + href: url, + asPath: resolvedAs, + locale: locale, + }), isServerRender: false, parseJSON: true, inflightCache: this.sdc, persistCache: !this.isPreview, isPrefetch: true, + unstable_skipClientCache: + options.unstable_skipClientCache || options.priority, }).then(() => false) : false }), diff --git a/test/e2e/prerender.test.ts b/test/e2e/prerender.test.ts index 813fa50b0e4f2..04e7a5fc58e17 100644 --- a/test/e2e/prerender.test.ts +++ b/test/e2e/prerender.test.ts @@ -336,12 +336,12 @@ describe('Prerender', () => { await goFromHomeToAnother() const nextTime = await browser.elementByCss('#anotherTime').text() + // in dev the time should always differ as we don't cache + // in production the time may differ or may not depending + // on if fresh content beat the stale content if (isDev) { expect(snapTime).not.toMatch(nextTime) - } else { - expect(snapTime).toMatch(nextTime) } - // Reset to Home for next test await goFromAnotherToHome() } diff --git a/test/integration/middleware-prefetch/tests/index.test.js b/test/integration/middleware-prefetch/tests/index.test.js index 796a78a1db5e4..67c3ae2b3f839 100644 --- a/test/integration/middleware-prefetch/tests/index.test.js +++ b/test/integration/middleware-prefetch/tests/index.test.js @@ -75,7 +75,6 @@ describe('Middleware Production Prefetch', () => { '/index.json', '/made-up.json', '/ssg-page-2.json', - '/ssg-page.json', ]) return 'yes' }, 'yes') diff --git a/test/lib/browsers/base.ts b/test/lib/browsers/base.ts index a456d884d3727..6a156d2fae796 100644 --- a/test/lib/browsers/base.ts +++ b/test/lib/browsers/base.ts @@ -46,6 +46,9 @@ export class BrowserInterface { type(text: string): BrowserInterface { return this } + moveTo(): BrowserInterface { + return this + } waitForElementByCss(selector: string, timeout?: number): BrowserInterface { return this } diff --git a/test/lib/next-modes/base.ts b/test/lib/next-modes/base.ts index ede007abdc39b..f6d2d9302260f 100644 --- a/test/lib/next-modes/base.ts +++ b/test/lib/next-modes/base.ts @@ -37,6 +37,7 @@ export class NextInstance { protected packageLockPath?: string protected basePath?: string protected env?: Record + public forcedPort?: string constructor({ files, diff --git a/test/lib/next-modes/next-dev.ts b/test/lib/next-modes/next-dev.ts index 88057494e77d8..f7161c7b7a9cf 100644 --- a/test/lib/next-modes/next-dev.ts +++ b/test/lib/next-modes/next-dev.ts @@ -38,7 +38,7 @@ export class NextDevInstance extends NextInstance { ...this.env, NODE_ENV: '' as any, __NEXT_TEST_MODE: '1', - __NEXT_RAND_PORT: '1', + __NEXT_FORCED_PORT: this.forcedPort || '0', __NEXT_TEST_WITH_DEVTOOL: '1', }, }) diff --git a/test/lib/next-modes/next-start.ts b/test/lib/next-modes/next-start.ts index 98f98c7614b4d..11515fce56ed3 100644 --- a/test/lib/next-modes/next-start.ts +++ b/test/lib/next-modes/next-start.ts @@ -49,7 +49,7 @@ export class NextStartInstance extends NextInstance { ...this.env, NODE_ENV: '' as any, __NEXT_TEST_MODE: '1', - __NEXT_RAND_PORT: '1', + __NEXT_FORCED_PORT: this.forcedPort || '0', }, } let buildArgs = ['yarn', 'next', 'build'] diff --git a/test/production/prerender-prefetch/index.test.ts b/test/production/prerender-prefetch/index.test.ts index 5e20a50a65171..dffeab9cc217c 100644 --- a/test/production/prerender-prefetch/index.test.ts +++ b/test/production/prerender-prefetch/index.test.ts @@ -1,6 +1,6 @@ import { NextInstance } from 'test/lib/next-modes/base' import { createNext, FileRef } from 'e2e-utils' -import { check, fetchViaHTTP, waitFor } from 'next-test-utils' +import { check, fetchViaHTTP, renderViaHTTP, waitFor } from 'next-test-utils' import cheerio from 'cheerio' import { join } from 'path' import webdriver from 'next-webdriver' @@ -88,4 +88,109 @@ describe('Prerender prefetch', () => { return 'success' }, 'success') }) + + it('should update cache using prefetch with unstable_skipClientCache', async () => { + const browser = await webdriver(next.url, '/') + const timeRes = await fetchViaHTTP( + next.url, + `/_next/data/${next.buildId}/blog/first.json`, + undefined, + { + headers: { + purpose: 'prefetch', + }, + } + ) + const startTime = (await timeRes.json()).pageProps.now + + // ensure stale data is used by default + await browser.elementByCss('#to-blog-first').click() + const outputIndex = next.cliOutput.length + + await check(() => browser.elementByCss('#page').text(), 'blog/[slug]') + + expect(JSON.parse(await browser.elementByCss('#props').text()).now).toBe( + startTime + ) + await browser.back().waitForElementByCss('#to-blog-first') + + // trigger revalidation of /blog/first + await check(async () => { + await renderViaHTTP(next.url, '/blog/first') + return next.cliOutput.substring(outputIndex) + }, /revalidating \/blog first/) + + // now trigger cache update and navigate again + await browser.eval( + 'next.router.prefetch("/blog/first", undefined, { unstable_skipClientCache: true }).finally(() => { window.prefetchDone = "yes" })' + ) + await check(() => browser.eval('window.prefetchDone'), 'yes') + + await browser.elementByCss('#to-blog-first').click() + await check(() => browser.elementByCss('#page').text(), 'blog/[slug]') + + const newTime = JSON.parse(await browser.elementByCss('#props').text()).now + expect(newTime).not.toBe(startTime) + expect(isNaN(newTime)).toBe(false) + }) + + it('should attempt cache update on link hover', async () => { + const browser = await webdriver(next.url, '/') + const timeRes = await fetchViaHTTP( + next.url, + `/_next/data/${next.buildId}/blog/first.json`, + undefined, + { + headers: { + purpose: 'prefetch', + }, + } + ) + const startTime = (await timeRes.json()).pageProps.now + + // ensure stale data is used by default + await browser.elementByCss('#to-blog-first').click() + await check(() => browser.elementByCss('#page').text(), 'blog/[slug]') + + expect(JSON.parse(await browser.elementByCss('#props').text()).now).toBe( + startTime + ) + await browser.back().waitForElementByCss('#to-blog-first') + const requests = [] + + browser.on('request', (req) => { + requests.push(req.url()) + }) + + // now trigger cache update and navigate again + await browser.elementByCss('#to-blog-first').moveTo() + await check( + () => + requests.some((url) => url.includes('/blog/first.json')) + ? 'success' + : requests, + 'success' + ) + }) + + it('should handle failed data fetch and empty cache correctly', async () => { + const browser = await webdriver(next.url, '/') + await browser.elementByCss('#to-blog-first').click() + + // ensure we use the same port when restarting + const port = new URL(next.url).port + next.forcedPort = port + + // trigger new build so buildId changes + await next.stop() + await next.start() + + // clear router cache + await browser.eval('window.next.router.sdc = {}') + await browser.eval('window.beforeNav = 1') + + await browser.back() + await browser.waitForElementByCss('#to-blog-first') + expect(await browser.eval('window.beforeNav')).toBeFalsy() + }) })