Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

only prefix prefetch cache entries if they vary based on Next-URL #61235

Merged
merged 4 commits into from
Feb 13, 2024

Conversation

ztanner
Copy link
Member

@ztanner ztanner commented Jan 26, 2024

What

Prefetches to pages within a shared layout would frequently cache miss despite having the data available. This causes the "instant navigation" behavior (with the 30s/5min TTL) to not be effective on these pages.

Why

In #59861, nextUrl was added as a prefetch cache key prefix to ensure multiple interception routes that correspond to the same URL wouldn't clash in the prefetch cache. However this causes a problem in the case where you're navigating between sub-pages. To illustrate the issue, consider the case where you load /foo. This will populate the prefetch cache with an entry of {foo: <PrefetchCacheNode}. Navigating to /foo/bar, with a link that prefetches back to /foo, will now result in a new cache node: {foo: <PrefetchCacheNode>, /foo/bar%/foo: <PrefetchCacheNode>} (where Next-URL is /foo/bar). Now we have a cache entry for the full data, as well as a cache entry for a partial prefetch up to the nearest loading boundary. Now when we navigate back to /foo, the router will see that it's missing data, and need to lazy-fetch the data triggering the loading boundary.

This was especially noticeable in the case where you have a route group with it's own loading.js file because it creates a level of hierarchy in the React tree, and suspending on the data fetch would result in the group's loading boundary to be triggered. In the non-route group scenario, there's still a bug here but it would stall on the data fetch rather than triggering a boundary.

How

In #61794 we conditionally send Next-URL as part of the Vary header if we detect it could be intercepted. We use this information when creating the prefetch entry to prefix it, in case it corresponds with an intercepted route.

Closes NEXT-2193

@ztanner ztanner force-pushed the 01-25-remove_unnecessary_PPR_branch_in_non-PPR_reducer branch from 02dcaa7 to 7f73ce6 Compare January 26, 2024 21:54
@ztanner ztanner force-pushed the 01-26-fix_erroneous_prefetch_cache_misses branch from 15a66e9 to a1a5802 Compare January 26, 2024 21:55
@ijjk
Copy link
Member

ijjk commented Jan 26, 2024

Failing test suites

Commit: 7df6f30

TURBOPACK=1 pnpm test-dev test/e2e/app-dir/not-found-default/index.test.ts (turbopack)

  • app dir - not found with default 404 page > should render default 404 with root layout for non-existent page
Expand output

● app dir - not found with default 404 page › should render default 404 with root layout for non-existent page

expect(received).not.toContain(expected) // indexOf

Expected substring: not "/_error"
Received string:        "yarn run v1.22.19
$ /tmp/next-install-4462d5af3cb7e3e8bbba0c6b972cce0291cac111cc464f3552c44bf97dbdd4dc/node_modules/.bin/next --turbo
   ▲ Next.js 14.1.1-canary.51 (turbo)
   - Local:        http://localhost:38683

Creating turbopack project {
  dir: '/tmp/next-install-4462d5af3cb7e3e8bbba0c6b972cce0291cac111cc464f3552c44bf97dbdd4dc',
  testMode: true
}
 ✓ Ready in 2.3s
 ○ Compiling / ...
 ✓ Compiled / in 1759ms
Error [ERR_HTTP_HEADERS_SENT]: Cannot set headers after they are sent to the client

  36 |         const cliOutput = next.cliOutput
  37 |         expect(cliOutput).toContain('/not-found')
> 38 |         expect(cliOutput).not.toContain('/_error')
     |                               ^
  39 |       }
  40 |     })
  41 |

  at handleRequest (../../../../../../tmp/next-install-4462d5af3cb7e3e8bbba0c6b972cce0291cac111cc464f3552c44bf97dbdd4dc/node_modules/.pnpm/file+..+next-repo-e7222500cc6ee46d1577d6e23bb8307685b2aba9f850586baa1ff8c73bfc1838+packages+n_3a25mllfnlolcpx6xdi7hmoyoi/node_modules/next/dist/server/lib/router-server.js:320:17)
  at async requestHandlerImpl (../../../../../../tmp/next-install-4462d5af3cb7e3e8bbba0c6b972cce0291cac111cc464f3552c44bf97dbdd4dc/node_modules/.pnpm/file+..+next-repo-e7222500cc6ee46d1577d6e23bb8307685b2aba9f850586baa1ff8c73bfc1838+packages+n_3a25mllfnlolcpx6xdi7hmoyoi/node_modules/next/dist/server/lib/router-server.js:339:13)
  at async Server.requestListener (../../../../../../tmp/next-install-4462d5af3cb7e3e8bbba0c6b972cce0291cac111cc464f3552c44bf97dbdd4dc/node_modules/.pnpm/file+..+next-repo-e7222500cc6ee46d1577d6e23bb8307685b2aba9f850586baa1ff8c73bfc1838+packages+n_3a25mllfnlolcpx6xdi7hmoyoi/node_modules/next/dist/server/lib/start-server.js:140:13) {
    code: 'ERR_HTTP_HEADERS_SENT'
  }
  Error [ERR_HTTP_HEADERS_SENT]: Cannot set headers after they are sent to the client
  at handleRequest (../../../../../../tmp/next-install-4462d5af3cb7e3e8bbba0c6b972cce0291cac111cc464f3552c44bf97dbdd4dc/node_modules/.pnpm/file+..+next-repo-e7222500cc6ee46d1577d6e23bb8307685b2aba9f850586baa1ff8c73bfc1838+packages+n_3a25mllfnlolcpx6xdi7hmoyoi/node_modules/next/dist/server/lib/router-server.js:320:17)
  at async requestHandlerImpl (../../../../../../tmp/next-install-4462d5af3cb7e3e8bbba0c6b972cce0291cac111cc464f3552c44bf97dbdd4dc/node_modules/.pnpm/file+..+next-repo-e7222500cc6ee46d1577d6e23bb8307685b2aba9f850586baa1ff8c73bfc1838+packages+n_3a25mllfnlolcpx6xdi7hmoyoi/node_modules/next/dist/server/lib/router-server.js:339:13)
  at async Server.requestListener (../../../../../../tmp/next-install-4462d5af3cb7e3e8bbba0c6b972cce0291cac111cc464f3552c44bf97dbdd4dc/node_modules/.pnpm/file+..+next-repo-e7222500cc6ee46d1577d6e23bb8307685b2aba9f850586baa1ff8c73bfc1838+packages+n_3a25mllfnlolcpx6xdi7hmoyoi/node_modules/next/dist/server/lib/start-server.js:140:13) {
    code: 'ERR_HTTP_HEADERS_SENT'
  }
   ○ Compiling /_error ...
   ✓ Compiled /_error in 668ms
   ✓ Compiled /not-found in 129ms
  "
  at Object.toContain (e2e/app-dir/not-found-default/index.test.ts:38:31)

Read more about building and testing Next.js in contributing.md.

@ijjk
Copy link
Member

ijjk commented Jan 26, 2024

Stats from current PR

Default Build (Increase detected ⚠️)
General Overall increase ⚠️
vercel/next.js canary vercel/next.js 01-26-fix_erroneous_prefetch_cache_misses Change
buildDuration 11.6s 11.8s ⚠️ +141ms
buildDurationCached 5.9s 5s N/A
nodeModulesSize 196 MB 196 MB ⚠️ +7.1 kB
nextStartRea..uration (ms) 427ms 439ms N/A
Client Bundles (main, webpack) Overall increase ⚠️
vercel/next.js canary vercel/next.js 01-26-fix_erroneous_prefetch_cache_misses Change
3f784ff6-HASH.js gzip 53.5 kB 53.5 kB N/A
423.HASH.js gzip 185 B 181 B N/A
68-HASH.js gzip 29.7 kB 29.8 kB ⚠️ +113 B
framework-HASH.js gzip 45.2 kB 45.2 kB
main-app-HASH.js gzip 238 B 239 B N/A
main-HASH.js gzip 31.9 kB 31.9 kB N/A
webpack-HASH.js gzip 1.7 kB 1.7 kB
Overall change 76.6 kB 76.7 kB ⚠️ +113 B
Legacy Client Bundles (polyfills)
vercel/next.js canary vercel/next.js 01-26-fix_erroneous_prefetch_cache_misses Change
polyfills-HASH.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages
vercel/next.js canary vercel/next.js 01-26-fix_erroneous_prefetch_cache_misses Change
_app-HASH.js gzip 194 B 195 B N/A
_error-HASH.js gzip 182 B 181 B N/A
amp-HASH.js gzip 502 B 501 B N/A
css-HASH.js gzip 320 B 322 B N/A
dynamic-HASH.js gzip 2.5 kB 2.5 kB N/A
edge-ssr-HASH.js gzip 255 B 256 B N/A
head-HASH.js gzip 350 B 349 B N/A
hooks-HASH.js gzip 368 B 369 B N/A
image-HASH.js gzip 4.2 kB 4.2 kB N/A
index-HASH.js gzip 257 B 256 B N/A
link-HASH.js gzip 2.67 kB 2.67 kB N/A
routerDirect..HASH.js gzip 310 B 311 B N/A
script-HASH.js gzip 384 B 383 B N/A
withRouter-HASH.js gzip 306 B 308 B N/A
1afbb74e6ecf..834.css gzip 106 B 106 B
Overall change 106 B 106 B
Client Build Manifests
vercel/next.js canary vercel/next.js 01-26-fix_erroneous_prefetch_cache_misses Change
_buildManifest.js gzip 483 B 484 B N/A
Overall change 0 B 0 B
Rendered Page Sizes
vercel/next.js canary vercel/next.js 01-26-fix_erroneous_prefetch_cache_misses Change
index.html gzip 527 B 527 B
link.html gzip 541 B 539 B N/A
withRouter.html gzip 523 B 522 B N/A
Overall change 527 B 527 B
Edge SSR bundle Size
vercel/next.js canary vercel/next.js 01-26-fix_erroneous_prefetch_cache_misses Change
edge-ssr.js gzip 94.4 kB 94.4 kB N/A
page.js gzip 150 kB 150 kB N/A
Overall change 0 B 0 B
Middleware size
vercel/next.js canary vercel/next.js 01-26-fix_erroneous_prefetch_cache_misses Change
middleware-b..fest.js gzip 621 B 624 B N/A
middleware-r..fest.js gzip 151 B 149 B N/A
middleware.js gzip 47.4 kB 47.4 kB N/A
edge-runtime..pack.js gzip 1.94 kB 1.94 kB
Overall change 1.94 kB 1.94 kB
Next Runtimes
vercel/next.js canary vercel/next.js 01-26-fix_erroneous_prefetch_cache_misses Change
app-page-exp...dev.js gzip 166 kB 166 kB
app-page-exp..prod.js gzip 95.4 kB 95.4 kB
app-page-tur..prod.js gzip 97.2 kB 97.2 kB
app-page-tur..prod.js gzip 91.6 kB 91.6 kB
app-page.run...dev.js gzip 136 kB 136 kB
app-page.run..prod.js gzip 90.2 kB 90.2 kB
app-route-ex...dev.js gzip 22 kB 22 kB
app-route-ex..prod.js gzip 14.9 kB 14.9 kB
app-route-tu..prod.js gzip 14.9 kB 14.9 kB
app-route-tu..prod.js gzip 14.6 kB 14.6 kB
app-route.ru...dev.js gzip 21.7 kB 21.7 kB
app-route.ru..prod.js gzip 14.6 kB 14.6 kB
pages-api-tu..prod.js gzip 9.43 kB 9.43 kB
pages-api.ru...dev.js gzip 9.7 kB 9.7 kB
pages-api.ru..prod.js gzip 9.43 kB 9.43 kB
pages-turbo...prod.js gzip 22.1 kB 22.1 kB
pages.runtim...dev.js gzip 22.7 kB 22.7 kB
pages.runtim..prod.js gzip 22.1 kB 22.1 kB
server.runti..prod.js gzip 49.9 kB 49.9 kB
Overall change 924 kB 924 kB
Diff details
Diff for page.js

Diff too large to display

Diff for 68-HASH.js

Diff too large to display

Commit: 7df6f30

@ztanner ztanner force-pushed the 01-26-fix_erroneous_prefetch_cache_misses branch 2 times, most recently from 2ee2f4e to fc0db8a Compare January 27, 2024 00:11
@ztanner ztanner force-pushed the 01-25-remove_unnecessary_PPR_branch_in_non-PPR_reducer branch from 7f73ce6 to e50ec2c Compare January 27, 2024 17:33
@ztanner ztanner force-pushed the 01-26-fix_erroneous_prefetch_cache_misses branch from fc0db8a to 3dd3a08 Compare January 27, 2024 17:33
@ztanner ztanner force-pushed the 01-25-remove_unnecessary_PPR_branch_in_non-PPR_reducer branch from e50ec2c to 85ef571 Compare January 27, 2024 18:05
@ztanner ztanner force-pushed the 01-26-fix_erroneous_prefetch_cache_misses branch 2 times, most recently from 4f48914 to 534f837 Compare January 29, 2024 22:23
@ztanner ztanner force-pushed the 01-25-remove_unnecessary_PPR_branch_in_non-PPR_reducer branch from 85ef571 to 8fbf043 Compare January 30, 2024 22:11
@ztanner ztanner force-pushed the 01-26-fix_erroneous_prefetch_cache_misses branch from 534f837 to 557dbe8 Compare January 30, 2024 23:18
packages/next/src/build/index.ts Outdated Show resolved Hide resolved
packages/next/src/server/web-server.ts Outdated Show resolved Hide resolved
@ztanner ztanner force-pushed the 01-26-fix_erroneous_prefetch_cache_misses branch from 557dbe8 to 916576c Compare January 30, 2024 23:50
@ztanner ztanner force-pushed the 01-25-remove_unnecessary_PPR_branch_in_non-PPR_reducer branch from 8fbf043 to 50b00d3 Compare January 31, 2024 01:25
@ztanner ztanner force-pushed the 01-26-fix_erroneous_prefetch_cache_misses branch from 916576c to 97b6040 Compare January 31, 2024 01:25
@ztanner ztanner marked this pull request as ready for review January 31, 2024 01:51
@ztanner ztanner force-pushed the 01-26-fix_erroneous_prefetch_cache_misses branch 3 times, most recently from 67494f0 to 4936d42 Compare February 7, 2024 21:11
@ztanner ztanner force-pushed the 02-07-consolidate_prefetch_utils branch from 2a94573 to b770e4e Compare February 7, 2024 21:46
@ztanner ztanner force-pushed the 01-26-fix_erroneous_prefetch_cache_misses branch from 4936d42 to e2c7fe9 Compare February 7, 2024 21:46
@ztanner ztanner force-pushed the 01-26-fix_erroneous_prefetch_cache_misses branch from e2c7fe9 to cd928f9 Compare February 7, 2024 23:31
@ztanner ztanner changed the base branch from 02-07-consolidate_prefetch_utils to 02-07-conditionally_send_Next-URL_in_Vary_response February 7, 2024 23:31
@ztanner ztanner force-pushed the 02-07-conditionally_send_Next-URL_in_Vary_response branch 2 times, most recently from 0f77d79 to 8bd70d5 Compare February 8, 2024 00:23
@ztanner ztanner changed the title fix erroneous prefetch cache misses only prefix prefetch cache entries if they vary based on Next-URL Feb 8, 2024
@ztanner ztanner force-pushed the 01-26-fix_erroneous_prefetch_cache_misses branch from cd928f9 to cd9837c Compare February 8, 2024 00:42
@ztanner ztanner force-pushed the 02-07-conditionally_send_Next-URL_in_Vary_response branch from 02c4657 to c329086 Compare February 9, 2024 16:59
Base automatically changed from 02-07-conditionally_send_Next-URL_in_Vary_response to canary February 9, 2024 17:57
ztanner added a commit that referenced this pull request Feb 9, 2024
To ensure that we properly cache data for routes that change based on `Next-URL` (which is used for route interception), this adjusts how we set the `Vary` header to conditionally include `Next-URL`. 

The `Next-URL` request header only impacts the response for routes that are intercepted. When we detect that path we're handling could be intercepted, we add `Next-URL` to the vary. This signals in #61235 to prefix these cache entries with `nextUrl` if the response might vary based on it. 

Closes NEXT-2398
@ztanner ztanner force-pushed the 01-26-fix_erroneous_prefetch_cache_misses branch 2 times, most recently from 040dd13 to b400940 Compare February 9, 2024 19:24
@ztanner ztanner force-pushed the 01-26-fix_erroneous_prefetch_cache_misses branch from b400940 to 778a4bb Compare February 9, 2024 19:27
@ztanner ztanner force-pushed the 01-26-fix_erroneous_prefetch_cache_misses branch from 778a4bb to 97b0a56 Compare February 9, 2024 19:30
Copy link
Contributor

@acdlite acdlite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's still slightly more changes than I would prefer to review in a single PR but I think it makes sense and I trust you :)

@ztanner ztanner enabled auto-merge (squash) February 13, 2024 14:42
@ztanner ztanner merged commit b9861fd into canary Feb 13, 2024
68 of 71 checks passed
@ztanner ztanner deleted the 01-26-fix_erroneous_prefetch_cache_misses branch February 13, 2024 15:03
@adrianha adrianha mentioned this pull request Feb 14, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants