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

conditionally send Next-URL in Vary response #61794

Merged
merged 2 commits into from
Feb 9, 2024

Conversation

ztanner
Copy link
Member

@ztanner ztanner commented Feb 7, 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

@ijjk
Copy link
Member

ijjk commented Feb 7, 2024

Stats from current PR

Default Build (Increase detected ⚠️)
General Overall increase ⚠️
vercel/next.js canary vercel/next.js 02-07-conditionally_send_Next-URL_in_Vary_response Change
buildDuration 11.8s 11.7s N/A
buildDurationCached 6s 5.3s N/A
nodeModulesSize 196 MB 196 MB ⚠️ +32 kB
nextStartRea..uration (ms) 432ms 435ms N/A
Client Bundles (main, webpack)
vercel/next.js canary vercel/next.js 02-07-conditionally_send_Next-URL_in_Vary_response 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.7 kB N/A
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.8 kB 31.8 kB N/A
webpack-HASH.js gzip 1.7 kB 1.7 kB
Overall change 46.9 kB 46.9 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary vercel/next.js 02-07-conditionally_send_Next-URL_in_Vary_response Change
polyfills-HASH.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages
vercel/next.js canary vercel/next.js 02-07-conditionally_send_Next-URL_in_Vary_response 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.19 kB 4.18 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 02-07-conditionally_send_Next-URL_in_Vary_response Change
_buildManifest.js gzip 483 B 485 B N/A
Overall change 0 B 0 B
Rendered Page Sizes
vercel/next.js canary vercel/next.js 02-07-conditionally_send_Next-URL_in_Vary_response Change
index.html gzip 527 B 526 B N/A
link.html gzip 541 B 537 B N/A
withRouter.html gzip 522 B 521 B N/A
Overall change 0 B 0 B
Edge SSR bundle Size Overall increase ⚠️
vercel/next.js canary vercel/next.js 02-07-conditionally_send_Next-URL_in_Vary_response Change
edge-ssr.js gzip 94 kB 94.2 kB ⚠️ +131 B
page.js gzip 150 kB 150 kB ⚠️ +148 B
Overall change 244 kB 244 kB ⚠️ +279 B
Middleware size
vercel/next.js canary vercel/next.js 02-07-conditionally_send_Next-URL_in_Vary_response Change
middleware-b..fest.js gzip 618 B 623 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 Overall increase ⚠️
vercel/next.js canary vercel/next.js 02-07-conditionally_send_Next-URL_in_Vary_response 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.7 kB 14.7 kB
app-route.ru...dev.js gzip 21.7 kB 21.7 kB
app-route.ru..prod.js gzip 14.7 kB 14.7 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 kB 22 kB
pages.runtim...dev.js gzip 22.7 kB 22.7 kB
pages.runtim..prod.js gzip 22 kB 22 kB
server.runti..prod.js gzip 49.7 kB 49.9 kB ⚠️ +197 B
Overall change 924 kB 924 kB ⚠️ +197 B
Diff details
Diff for page.js

Diff too large to display

Diff for middleware.js

Diff too large to display

Diff for edge-ssr.js

Diff too large to display

Diff for 68-HASH.js

Diff too large to display

Diff for server.runtime.prod.js

Diff too large to display

Commit: c329086

@ztanner ztanner force-pushed the 02-07-conditionally_send_Next-URL_in_Vary_response branch from dd262b1 to 0f77d79 Compare February 8, 2024 00:14
@ijjk
Copy link
Member

ijjk commented Feb 8, 2024

Tests Passed

@@ -5,8 +5,6 @@ export const NEXT_ROUTER_STATE_TREE = 'Next-Router-State-Tree' as const
export const NEXT_ROUTER_PREFETCH_HEADER = 'Next-Router-Prefetch' as const
export const NEXT_URL = 'Next-Url' as const
export const RSC_CONTENT_TYPE_HEADER = 'text/x-component' as const
export const RSC_VARY_HEADER =
Copy link
Member Author

Choose a reason for hiding this comment

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

This is removed as a constant to avoid any assumptions about the Vary header being static. Instead we inline the vary header in places where we're setting it.

Base automatically changed from 02-07-consolidate_prefetch_utils to canary February 8, 2024 00:22
@ztanner ztanner force-pushed the 02-07-conditionally_send_Next-URL_in_Vary_response branch from 0f77d79 to 8bd70d5 Compare February 8, 2024 00:23
@ztanner ztanner marked this pull request as ready for review February 8, 2024 00:41
packages/next/src/server/base-server.ts Show resolved Hide resolved
packages/next/src/server/base-server.ts Outdated Show resolved Hide resolved
packages/next/src/server/base-server.ts Outdated Show resolved Hide resolved
packages/next/src/server/base-server.ts Show resolved Hide resolved
@ztanner ztanner force-pushed the 02-07-conditionally_send_Next-URL_in_Vary_response branch from 57ea554 to 02c4657 Compare February 9, 2024 16:09
@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
@ztanner ztanner merged commit 12bfa97 into canary Feb 9, 2024
70 checks passed
@ztanner ztanner deleted the 02-07-conditionally_send_Next-URL_in_Vary_response branch February 9, 2024 17:57
ztanner added a commit that referenced this pull request Feb 9, 2024
In #61794, the routes manifest is used to find the interception route rewrites in `next-server` and computed on the fly in `next-dev-server` based on `appPaths`.

The edge runtime doesn't have access to the routes manifest nor a full list of app paths. This writes an entry for the edge runtime to make the interception routes readable, and adds plumbing to return them in the `getInterceptionRouteRewrites` handling in `web-server`. This is what we use to signal to the server whether to return ‘Next-URL’ in the Vary for RSC requests. 

This piggybacks on the existing interception routes test but adds an edge runtime case.

Closes NEXT-2304
@darthmaim
Copy link

@ztanner since this changes some logic around setting the vary header, would fixing #48480 be possible as well?

@adrianha adrianha mentioned this pull request Feb 12, 2024
EndangeredMassa added a commit to vercel/vercel that referenced this pull request Feb 13, 2024
The Next.js change in vercel/next.js#61794 means
that the `Vary` header will contain `Next-Url` less often. We need to
update the assertions on our end to accommodate.
ztanner added a commit that referenced this pull request Feb 13, 2024
…61235)

### 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
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 24, 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.

None yet

4 participants