Skip to content

Commit

Permalink
Fix hydration middleware effects (vercel#31800)
Browse files Browse the repository at this point in the history
Whenever we trigger a route change in the client we check if there route we are navigating to is affected by a middleware. When this is the case we run a preflight and in case there is an effect that tells us that the middleware is responding with content we force a _refresh_. This is fine for navigation in general but it is not ok when the change is triggered for hydration. For example, in cases where the rendered page is a dynamic page this triggers an infinite reload.

In this PR we add a test where we add a `_middleware` that proxies to a dynamic path. When making a request to `/interface/root-subrequest` there will be a middleware that simply performs a fetch against localhost for the same `/interface/root-subrequest`. The new request will skip the middleware to avoid loops and then render the dynamic page. Then client will force a change for hydration resulting in a preflight request that tells that the client must refresh because for that path there is a middleware writing content.

Then we add a fix which simply consist of checking the internal option that tells when a change is triggered for hydration and skip the preflight in such scenario.

## Bug

- [ ] Related issues linked using `fixes #number`
- [x] Integration tests added
- [ ] Errors have helpful link attached, see `contributing.md`

## Feature

- [ ] Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR.
- [ ] Related issues linked using `fixes #number`
- [ ] Integration tests added
- [ ] Documentation added
- [ ] Telemetry added. In case of a feature if it's used or not.
- [ ] Errors have helpful link attached, see `contributing.md`

## Documentation / Examples

- [ ] Make sure the linting passes by running `yarn lint`
  • Loading branch information
javivelasco authored Nov 26, 2021
1 parent e52bee3 commit 2e5218c
Show file tree
Hide file tree
Showing 3 changed files with 43 additions and 21 deletions.
49 changes: 28 additions & 21 deletions packages/next/shared/lib/router/router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1133,28 +1133,35 @@ export default class Router implements BaseRouter {

resolvedAs = delLocale(delBasePath(resolvedAs), this.locale)

const effect = await this._preflightRequest({
as,
cache: process.env.NODE_ENV === 'production',
pages,
pathname,
query,
})
/**
* If the route update was triggered for client-side hydration then
* do not check the preflight request. Otherwise when rendering
* a page with refresh it might get into an infinite loop.
*/
if ((options as any)._h !== 1) {
const effect = await this._preflightRequest({
as,
cache: process.env.NODE_ENV === 'production',
pages,
pathname,
query,
})

if (effect.type === 'rewrite') {
query = { ...query, ...effect.parsedAs.query }
resolvedAs = effect.asPath
pathname = effect.resolvedHref
parsed.pathname = effect.resolvedHref
url = formatWithValidation(parsed)
} else if (effect.type === 'redirect' && effect.newAs) {
return this.change(method, effect.newUrl, effect.newAs, options)
} else if (effect.type === 'redirect' && effect.destination) {
window.location.href = effect.destination
return new Promise(() => {})
} else if (effect.type === 'refresh') {
window.location.href = as
return new Promise(() => {})
if (effect.type === 'rewrite') {
query = { ...query, ...effect.parsedAs.query }
resolvedAs = effect.asPath
pathname = effect.resolvedHref
parsed.pathname = effect.resolvedHref
url = formatWithValidation(parsed)
} else if (effect.type === 'redirect' && effect.newAs) {
return this.change(method, effect.newUrl, effect.newAs, options)
} else if (effect.type === 'redirect' && effect.destination) {
window.location.href = effect.destination
return new Promise(() => {})
} else if (effect.type === 'refresh') {
window.location.href = as
return new Promise(() => {})
}
}

const route = removePathTrailingSlash(pathname)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,12 @@ export async function middleware(request) {
}
}

if (url.pathname.endsWith('/root-subrequest')) {
return fetch(
`http://${request.headers.get('host')}/interface/root-subrequest`
)
}

return new Response(null, {
headers: {
'req-url-basepath': request.nextUrl.basePath,
Expand Down
9 changes: 9 additions & 0 deletions test/integration/middleware/core/test/index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -438,6 +438,15 @@ function interfaceTests(locale = '') {
expect(res.headers.get('req-url-locale')).toBe(locale.slice(1))
}
})

it(`${locale} renders correctly rewriting with a root subrequest`, async () => {
const browser = await webdriver(
context.appPort,
'/interface/root-subrequest'
)
const element = await browser.elementByCss('.title')
expect(await element.text()).toEqual('Dynamic route')
})
}

function getCookieFromResponse(res, cookieName) {
Expand Down

0 comments on commit 2e5218c

Please sign in to comment.