From e2e0644f515a0cd143be5821e3515e5a301c6db8 Mon Sep 17 00:00:00 2001 From: Zack Tanner <1939140+ztanner@users.noreply.github.com> Date: Wed, 21 Aug 2024 11:38:55 -0700 Subject: [PATCH] bugfix: ensure prefetch aliasing doesn't apply URL redirect (#69153) Another follow up to #68340: When a prefetch is aliased (meaning we returned a prefetch entry that corresponded with the same pathname, but different / missing search params), we signal to the router that it should only copy the loading state from this prefetch entry, and null everything else out so it can be fetched from the server. However it's possible that we return an aliased entry that corresponds with a redirect (such as in middleware) meaning the server will supply a `canonicalUrlOverride` value that will correspond with the aliased entry, rather than the requested entry. This updates the handling to only apply the canonical URL override when we're not using an aliased entry. This will defer the redirect handling to when the actual dynamic request (in the server patch reducer) so that the aliased entry will only handle the loading copying and not do any redirects. This copies the existing tests that covered this behavior but adds a middleware redirect to verify the behavior is the same. --- .../reducers/navigate-reducer.ts | 12 +- .../app/with-middleware/page.tsx | 28 +++ .../with-middleware/search-params/loading.tsx | 3 + .../search-params/someValue/page.tsx | 16 ++ .../searchparams-reuse-loading/middleware.ts | 26 +++ .../searchparams-reuse-loading.test.ts | 197 ++++++++++-------- 6 files changed, 189 insertions(+), 93 deletions(-) create mode 100644 test/e2e/app-dir/searchparams-reuse-loading/app/with-middleware/page.tsx create mode 100644 test/e2e/app-dir/searchparams-reuse-loading/app/with-middleware/search-params/loading.tsx create mode 100644 test/e2e/app-dir/searchparams-reuse-loading/app/with-middleware/search-params/someValue/page.tsx create mode 100644 test/e2e/app-dir/searchparams-reuse-loading/middleware.ts diff --git a/packages/next/src/client/components/router-reducer/reducers/navigate-reducer.ts b/packages/next/src/client/components/router-reducer/reducers/navigate-reducer.ts index 24238c27cb84f..08f6ef77acbeb 100644 --- a/packages/next/src/client/components/router-reducer/reducers/navigate-reducer.ts +++ b/packages/next/src/client/components/router-reducer/reducers/navigate-reducer.ts @@ -148,9 +148,15 @@ export function navigateReducer( return handleExternalUrl(state, mutable, href, pendingPush) } - const updatedCanonicalUrl = canonicalUrlOverride - ? createHrefFromUrl(canonicalUrlOverride) - : href + // When the server indicates an override for the canonical URL (such as a redirect in middleware) + // we only want to use that if we're not using an aliased entry as the redirect will correspond with + // the aliased prefetch which might have different search params. Since we're only using the aliased entry + // for the loading state, the proper override will happen in the server patch action when the dynamic + // data is loaded. + const updatedCanonicalUrl = + canonicalUrlOverride && !prefetchValues.aliased + ? createHrefFromUrl(canonicalUrlOverride) + : href // Track if the navigation was only an update to the hash fragment mutable.onlyHashChange = diff --git a/test/e2e/app-dir/searchparams-reuse-loading/app/with-middleware/page.tsx b/test/e2e/app-dir/searchparams-reuse-loading/app/with-middleware/page.tsx new file mode 100644 index 0000000000000..890256e1d024a --- /dev/null +++ b/test/e2e/app-dir/searchparams-reuse-loading/app/with-middleware/page.tsx @@ -0,0 +1,28 @@ +import Link from 'next/link' + +export default function Page() { + return ( + + ) +} diff --git a/test/e2e/app-dir/searchparams-reuse-loading/app/with-middleware/search-params/loading.tsx b/test/e2e/app-dir/searchparams-reuse-loading/app/with-middleware/search-params/loading.tsx new file mode 100644 index 0000000000000..1d7bf4a78bfc6 --- /dev/null +++ b/test/e2e/app-dir/searchparams-reuse-loading/app/with-middleware/search-params/loading.tsx @@ -0,0 +1,3 @@ +export default function Loading() { + return

Loading...

+} diff --git a/test/e2e/app-dir/searchparams-reuse-loading/app/with-middleware/search-params/someValue/page.tsx b/test/e2e/app-dir/searchparams-reuse-loading/app/with-middleware/search-params/someValue/page.tsx new file mode 100644 index 0000000000000..4c40b77c01014 --- /dev/null +++ b/test/e2e/app-dir/searchparams-reuse-loading/app/with-middleware/search-params/someValue/page.tsx @@ -0,0 +1,16 @@ +import Link from 'next/link' + +export default async function Page({ + searchParams, +}: { + searchParams: Record +}) { + // sleep for 500ms + await new Promise((resolve) => setTimeout(resolve, 500)) + return ( + <> +

{JSON.stringify(searchParams)}

+ Back + + ) +} diff --git a/test/e2e/app-dir/searchparams-reuse-loading/middleware.ts b/test/e2e/app-dir/searchparams-reuse-loading/middleware.ts new file mode 100644 index 0000000000000..a82f0c53d2ea9 --- /dev/null +++ b/test/e2e/app-dir/searchparams-reuse-loading/middleware.ts @@ -0,0 +1,26 @@ +import { NextResponse } from 'next/server' +import type { NextRequest } from 'next/server' + +export function middleware(request: NextRequest) { + const { pathname, search } = request.nextUrl + + if ( + pathname.startsWith('/with-middleware/search-params') && + !pathname.includes('someValue') + ) { + const newPathname = pathname.replace( + '/with-middleware/search-params', + '/with-middleware/search-params/someValue' + ) + console.log('performing redirect') + return NextResponse.redirect( + new URL(`${newPathname}${search}`, request.url) + ) + } + + return NextResponse.next() +} + +export const config = { + matcher: '/with-middleware/search-params', +} diff --git a/test/e2e/app-dir/searchparams-reuse-loading/searchparams-reuse-loading.test.ts b/test/e2e/app-dir/searchparams-reuse-loading/searchparams-reuse-loading.test.ts index 6a6dbbf764200..c0d20cca141cc 100644 --- a/test/e2e/app-dir/searchparams-reuse-loading/searchparams-reuse-loading.test.ts +++ b/test/e2e/app-dir/searchparams-reuse-loading/searchparams-reuse-loading.test.ts @@ -34,99 +34,116 @@ describe('searchparams-reuse-loading', () => { // Dev doesn't perform prefetching, so this test is skipped, as it relies on intercepting // prefetch network requests. if (!isNextDev) { - it('should correctly return different RSC data for full prefetches with different searchParam values', async () => { - const rscRequestPromise = new Map< - string, - { resolve: () => Promise } - >() - - let interceptRequests = false - const browser = await next.browser('/', { - beforePageLoad(page: Page) { - page.route('**/search-params*', async (route: Route) => { - if (!interceptRequests) { - return route.continue() - } - - const request = route.request() - const headers = await request.allHeaders() - const url = new URL(request.url()) - const promiseKey = - url.pathname + '?id=' + url.searchParams.get('id') - - if (headers['rsc'] === '1' && !headers['next-router-prefetch']) { - // Create a promise that will be resolved by the later test code - let resolvePromise: () => void - const promise = new Promise((res) => { - resolvePromise = res - }) - - if (rscRequestPromise.has(promiseKey)) { - throw new Error('Duplicate request') + describe.each([ + { path: '/', label: 'Without Middleware' }, + { path: '/with-middleware', label: 'With Middleware' }, + ])('$label', ({ path }) => { + it('should correctly return different RSC data for full prefetches with different searchParam values', async () => { + const rscRequestPromise = new Map< + string, + { resolve: () => Promise } + >() + + let interceptRequests = false + const browser = await next.browser(path, { + beforePageLoad(page: Page) { + page.route('**/search-params*', async (route: Route) => { + if (!interceptRequests) { + return route.continue() } - rscRequestPromise.set(promiseKey, { - resolve: async () => { - await route.continue() - // wait a moment to ensure the response is received - await new Promise((res) => setTimeout(res, 500)) - resolvePromise() - }, - }) - - // Await the promise to effectively stall the request - await promise - } else { - await route.continue() - } - }) - }, + const request = route.request() + const headers = await request.allHeaders() + const url = new URL(request.url()) + const promiseKey = + url.pathname + '?id=' + url.searchParams.get('id') + + if (headers['rsc'] === '1' && !headers['next-router-prefetch']) { + // Create a promise that will be resolved by the later test code + let resolvePromise: () => void + const promise = new Promise((res) => { + resolvePromise = res + }) + + if (rscRequestPromise.has(promiseKey)) { + throw new Error('Duplicate request') + } + + rscRequestPromise.set(promiseKey, { + resolve: async () => { + await route.continue() + // wait a moment to ensure the response is received + await new Promise((res) => setTimeout(res, 500)) + resolvePromise() + }, + }) + + // Await the promise to effectively stall the request + await promise + } else { + await route.continue() + } + }) + }, + }) + + const basePath = path === '/' ? '' : path + + await browser.waitForIdleNetwork() + interceptRequests = true + // The first link we click is "auto" prefetched. + await browser + .elementByCss(`[href="${basePath}/search-params?id=1"]`) + .click() + + // We expect to click it and immediately see a loading state + expect(await browser.elementById('loading').text()).toBe('Loading...') + // We only resolve the dynamic request after we've confirmed loading exists, + // to avoid a race where the dynamic request handles the loading state instead. + let dynamicRequest = rscRequestPromise.get( + `${basePath}/search-params?id=1` + ) + expect(dynamicRequest).toBeDefined() + + // resolve the promise + await dynamicRequest.resolve() + dynamicRequest = undefined + + // Confirm the params are correct + const params = await browser.waitForElementByCss('#params').text() + expect(params).toBe('{"id":"1"}') + + await browser.elementByCss(`[href='${path}']`).click() + + // Do the exact same thing again, for another prefetch auto link, to ensure + // loading works as expected and we get different search params + await browser + .elementByCss(`[href="${basePath}/search-params?id=2"]`) + .click() + expect(await browser.elementById('loading').text()).toBe('Loading...') + dynamicRequest = rscRequestPromise.get(`${basePath}/search-params?id=2`) + expect(dynamicRequest).toBeDefined() + + // resolve the promise + await dynamicRequest.resolve() + dynamicRequest = undefined + + const params2 = await browser.waitForElementByCss('#params').text() + expect(params2).toBe('{"id":"2"}') + + // Dev mode doesn't perform full prefetches, so this test is conditional + await browser.elementByCss(`[href='${path}']`).click() + + await browser + .elementByCss(`[href="${basePath}/search-params?id=3"]`) + .click() + expect(rscRequestPromise.has(`${basePath}/search-params?id=3`)).toBe( + false + ) + // no need to resolve any dynamic requests, as this is a full prefetch + const params3 = await browser.waitForElementByCss('#params').text() + expect(params3).toBe('{"id":"3"}') }) - - await browser.waitForIdleNetwork() - interceptRequests = true - // The first link we click is "auto" prefetched. - await browser.elementByCss('[href="/search-params?id=1"]').click() - - // We expect to click it and immediately see a loading state - expect(await browser.elementById('loading').text()).toBe('Loading...') - // We only resolve the dynamic request after we've confirmed loading exists, - // to avoid a race where the dynamic request handles the loading state instead. - let dynamicRequest = rscRequestPromise.get('/search-params?id=1') - expect(dynamicRequest).toBeDefined() - - // resolve the promise - await dynamicRequest.resolve() - dynamicRequest = undefined - - // Confirm the params are correct - const params = await browser.waitForElementByCss('#params').text() - expect(params).toBe('{"id":"1"}') - - await browser.elementByCss("[href='/']").click() - - // Do the exact same thing again, for another prefetch auto link, to ensure - // loading works as expected and we get different search params - await browser.elementByCss('[href="/search-params?id=2"]').click() - expect(await browser.elementById('loading').text()).toBe('Loading...') - dynamicRequest = rscRequestPromise.get('/search-params?id=2') - expect(dynamicRequest).toBeDefined() - - // resolve the promise - await dynamicRequest.resolve() - dynamicRequest = undefined - - const params2 = await browser.waitForElementByCss('#params').text() - expect(params2).toBe('{"id":"2"}') - - // Dev mode doesn't perform full prefetches, so this test is conditional - await browser.elementByCss("[href='/']").click() - - await browser.elementByCss('[href="/search-params?id=3"]').click() - expect(rscRequestPromise.has('/search-params?id=3')).toBe(false) - // no need to resolve any dynamic requests, as this is a full prefetch - const params3 = await browser.waitForElementByCss('#params').text() - expect(params3).toBe('{"id":"3"}') }) // /search-params (full) to /search-params?id=1 (missing)