Skip to content

Commit

Permalink
Fix hard navigation guard on popstate and handle fetching fresh data (#…
Browse files Browse the repository at this point in the history 
…37970)

* Fix hard navigation guard on popstate and handle fetching fresh data

* undo link changes and handle middleware cases

* update tests
  • Loading branch information
@ijjk
ijjk committed Jun 24, 2022
1 parent 07a6d4a commit edd798e
Show file tree
Hide file tree
Showing 10 changed files with 235 additions and 91 deletions.
4 changes: 2 additions & 2 deletions packages/next/cli/next-dev.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions packages/next/cli/next-start.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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({
Expand Down
196 changes: 116 additions & 80 deletions packages/next/shared/lib/router/router.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ interface TransitionOptions {
shallow?: boolean
locale?: string | false
scroll?: boolean
unstable_skipClientCache?: boolean
}

interface NextHistoryState {
Expand Down Expand Up @@ -339,6 +340,7 @@ export type NextRouter = BaseRouter &
export type PrefetchOptions = {
priority?: boolean
locale?: string | false
unstable_skipClientCache?: boolean
}

export type PrivateRouteInfo =
Expand Down Expand Up @@ -437,6 +439,7 @@ interface FetchNextDataParams {
persistCache: boolean
isPrefetch: boolean
isBackground?: boolean
unstable_skipClientCache?: boolean
}

function fetchNextData({
Expand All @@ -448,6 +451,7 @@ function fetchNextData({
parseJSON,
persistCache,
isBackground,
unstable_skipClientCache,
}: FetchNextDataParams): Promise<FetchDataOutput> {
const { href: cacheKey } = new URL(dataHref, window.location.href)
const getData = (params?: { method?: 'HEAD' | 'GET' }) =>
Expand All @@ -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]
}
Expand All @@ -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}`
)
Expand Down Expand Up @@ -949,7 +962,7 @@ export default class Router implements BaseRouter {
forcedScroll?: { x: number; y: number }
): Promise<boolean> {
if (!isLocalURL(url)) {
handleHardNavigation({ url })
handleHardNavigation({ url, router: this })
return false
}
// WARNING: `_h` is an internal option for handing Next.js client-side
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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(() => {})
}
}
Expand Down Expand Up @@ -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(() => {})
}

Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -1613,6 +1633,7 @@ export default class Router implements BaseRouter {
locale,
hasMiddleware,
isPreview,
unstable_skipClientCache,
}: {
route: string
pathname: string
Expand All @@ -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
Expand Down Expand Up @@ -1664,6 +1686,7 @@ export default class Router implements BaseRouter {
inflightCache: this.sdc,
persistCache: !isPreview,
isPrefetch: false,
unstable_skipClientCache,
}

const data = await withMiddlewareEffects({
Expand Down Expand Up @@ -1758,6 +1781,7 @@ export default class Router implements BaseRouter {
inflightCache: this.sdc,
persistCache: !isPreview,
isPrefetch: false,
unstable_skipClientCache,
}))

return {
Expand Down Expand Up @@ -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)
}
}
Expand All @@ -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({
Expand Down Expand Up @@ -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
}),
Expand Down
Loading

0 comments on commit edd798e

Please sign in to comment.