[Segment Cache] Move cache key creation to client #73853

acdlite · 2024-12-12T17:32:20Z

Based on:

[Segment Cache] Add CacheStatus.Empty #73667

When I originally set up the Segment Cache transport types, I had the notion that the client should never have to compute the request key that is sent to the server. That way we could, say, encrypt the keys on the server to make them unguessable.

But once we added the concept of access tokens for that purpose, this motivation was gone. The remaining motivation was to keep the contract as simple as possible.

However, there's a lot of existing code that relies on manipulating the "decoded" form of the segment values. Since we're sending the full segment value anyway, it's wasteful to also send the encoded form, given that it can be trivially re-derived by the client.

My immediate motivation is that, I need to be able to construct a cache key from a FlightRouterState. As a bonus, this will also get us closer to being able to remove the dynamic param values from the server response, a separate goal that we're currently working on.

We can revisit this once if/when we're able to remove the existing cases where the full segment is needed by the client.

acdlite · 2024-12-12T17:35:40Z

packages/next/src/server/app-render/segment-value-encoding.ts

+  const safeName = encodeToFilesystemAndURLSafeString(name)
+  const safeValue = encodeToFilesystemAndURLSafeString(paramValue)
+
+  const encodedName = '$' + paramType + '$' + safeName + '$' + safeValue


I changed the encoding a bit here in anticipation of our upcoming project to remove the dynamic params from the server response and instead extract them from the URL on the client. The server will send everything except for that final safeValue. And then the client will append it immediately when processing the payload.

There's a comment about that here: https://github.com/vercel/next.js/pull/73853/files#diff-f296fd6b5d4b377330673eab8efc9709ba5dfbcde46a1dffa1d76adf400717c8R506-R509

ijjk · 2024-12-12T17:36:41Z

Failing test suites

Commit: 0ea36b0

pnpm test test/integration/build-warnings/test/index.test.js (turbopack)

Build warnings > production mode > should warn about missing cache in CI

Expand output

● Build warnings › production mode › should warn about missing cache in CI

thrown: "Exceeded timeout of 60000 ms for a test.
Add a timeout value to this test to increase the timeout, if this is a long-running test. See https://jestjs.io/docs/api#testname-fn-timeout."

  84 |       })
  85 |
> 86 |       it('should warn about missing cache in CI', async () => {
     |       ^
  87 |         await remove(join(appDir, '.next'))
  88 |
  89 |         let { stdout } = await nextBuild(appDir, undefined, {

  at it (integration/build-warnings/test/index.test.js:86:7)
  at integration/build-warnings/test/index.test.js:10:56
  at Object.describe (integration/build-warnings/test/index.test.js:9:1)

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

ijjk · 2024-12-12T17:40:26Z

Stats from current PR

Default Build (Increase detected ⚠️)

General Overall increase ⚠️

	vercel/next.js canary	acdlite/next.js move-cache-key-creation	Change
buildDuration	17.3s	15.6s	N/A
buildDurationCached	14.8s	12.5s	N/A
nodeModulesSize	410 MB	410 MB	⚠️ +100 kB
nextStartRea..uration (ms)	478ms	475ms	N/A

Client Bundles (main, webpack) Overall increase ⚠️

	vercel/next.js canary	acdlite/next.js move-cache-key-creation	Change
1187-HASH.js gzip	51.1 kB	51.4 kB	⚠️ +293 B
8276.HASH.js gzip	169 B	168 B	N/A
8377-HASH.js gzip	5.36 kB	5.36 kB	N/A
bccd1874-HASH.js gzip	53 kB	53 kB	N/A
framework-HASH.js gzip	57.5 kB	57.5 kB	N/A
main-app-HASH.js gzip	232 B	236 B	N/A
main-HASH.js gzip	34.1 kB	34 kB	N/A
webpack-HASH.js gzip	1.71 kB	1.71 kB	N/A
Overall change	51.1 kB	51.4 kB	⚠️ +293 B

Legacy Client Bundles (polyfills)

	vercel/next.js canary	acdlite/next.js move-cache-key-creation	Change
polyfills-HASH.js gzip	39.4 kB	39.4 kB	✓
Overall change	39.4 kB	39.4 kB	✓

Client Pages

	vercel/next.js canary	acdlite/next.js move-cache-key-creation	Change
_app-HASH.js gzip	193 B	193 B	✓
_error-HASH.js gzip	193 B	193 B	✓
amp-HASH.js gzip	512 B	510 B	N/A
css-HASH.js gzip	343 B	342 B	N/A
dynamic-HASH.js gzip	1.84 kB	1.84 kB	✓
edge-ssr-HASH.js gzip	265 B	265 B	✓
head-HASH.js gzip	363 B	362 B	N/A
hooks-HASH.js gzip	393 B	392 B	N/A
image-HASH.js gzip	4.49 kB	4.49 kB	N/A
index-HASH.js gzip	268 B	268 B	✓
link-HASH.js gzip	2.35 kB	2.34 kB	N/A
routerDirect..HASH.js gzip	328 B	328 B	✓
script-HASH.js gzip	397 B	397 B	✓
withRouter-HASH.js gzip	323 B	326 B	N/A
1afbb74e6ecf..834.css gzip	106 B	106 B	✓
Overall change	3.59 kB	3.59 kB	✓

Client Build Manifests

	vercel/next.js canary	acdlite/next.js move-cache-key-creation	Change
_buildManifest.js gzip	749 B	746 B	N/A
Overall change	0 B	0 B	✓

Rendered Page Sizes

	vercel/next.js canary	acdlite/next.js move-cache-key-creation	Change
index.html gzip	524 B	523 B	N/A
link.html gzip	539 B	537 B	N/A
withRouter.html gzip	520 B	519 B	N/A
Overall change	0 B	0 B	✓

Edge SSR bundle Size

	vercel/next.js canary	acdlite/next.js move-cache-key-creation	Change
edge-ssr.js gzip	128 kB	128 kB	N/A
page.js gzip	204 kB	204 kB	N/A
Overall change	0 B	0 B	✓

Middleware size

	vercel/next.js canary	acdlite/next.js move-cache-key-creation	Change
middleware-b..fest.js gzip	672 B	666 B	N/A
middleware-r..fest.js gzip	155 B	156 B	N/A
middleware.js gzip	31.2 kB	31.2 kB	N/A
edge-runtime..pack.js gzip	844 B	844 B	✓
Overall change	844 B	844 B	✓

Next Runtimes Overall increase ⚠️

	vercel/next.js canary	acdlite/next.js move-cache-key-creation	Change
523-experime...dev.js gzip	322 B	322 B	✓
523.runtime.dev.js gzip	314 B	314 B	✓
app-page-exp...dev.js gzip	323 kB	324 kB	⚠️ +395 B
app-page-exp..prod.js gzip	127 kB	128 kB	⚠️ +222 B
app-page-tur..prod.js gzip	140 kB	141 kB	⚠️ +218 B
app-page-tur..prod.js gzip	135 kB	136 kB	⚠️ +220 B
app-page.run...dev.js gzip	314 kB	314 kB	⚠️ +399 B
app-page.run..prod.js gzip	123 kB	124 kB	⚠️ +226 B
app-route-ex...dev.js gzip	37.4 kB	37.4 kB	✓
app-route-ex..prod.js gzip	25.5 kB	25.5 kB	✓
app-route-tu..prod.js gzip	25.5 kB	25.5 kB	✓
app-route-tu..prod.js gzip	25.3 kB	25.3 kB	✓
app-route.ru...dev.js gzip	39 kB	39 kB	✓
app-route.ru..prod.js gzip	25.3 kB	25.3 kB	✓
pages-api-tu..prod.js gzip	9.69 kB	9.69 kB	✓
pages-api.ru...dev.js gzip	11.6 kB	11.6 kB	✓
pages-api.ru..prod.js gzip	9.68 kB	9.68 kB	✓
pages-turbo...prod.js gzip	21.7 kB	21.7 kB	✓
pages.runtim...dev.js gzip	27.4 kB	27.4 kB	✓
pages.runtim..prod.js gzip	21.7 kB	21.7 kB	✓
server.runti..prod.js gzip	916 kB	916 kB	✓
Overall change	2.36 MB	2.36 MB	⚠️ +1.68 kB

build cache

	vercel/next.js canary	acdlite/next.js move-cache-key-creation	Change
0.pack gzip	2.06 MB	2.05 MB	N/A
index.pack gzip	72.9 kB	71.3 kB	N/A
Overall change	0 B	0 B	✓

Diff details

Diff for 1187-HASH.js

Diff too large to display

Diff for main-HASH.js

Diff too large to display

Diff for app-page-exp..ntime.dev.js

Diff too large to display

Diff for app-page-exp..time.prod.js

Diff too large to display

Diff for app-page-tur..time.prod.js

Diff too large to display

Diff for app-page-tur..time.prod.js

Diff too large to display

Diff for app-page.runtime.dev.js

Diff too large to display

Diff for app-page.runtime.prod.js

Diff too large to display

Commit: 0ea36b0

ztanner · 2024-12-16T23:06:46Z

test/e2e/app-dir/ppr-navigations/simple/per-segment-prefetching.test.ts

-    // Confirm that the prefetch was successful. This is a basic check to ensure
-    // that the name of an expected field is somewhere in the Flight stream.
-    expect(childResponseText).toInclude('"rsc"')


how come we no longer assert on this behavior?

So that it's not coupled to the cache key generation algorithm, which happened to change in this PPR. Really we could just delete this whole test. The original test was written before any of the client behavior was implemented; at this point it's more than covered by the e2e tests.

When I originally set up the Segment Cache transport types, I had the notion that the client should never have to compute the request key that is sent to the server. That way we could, say, encrypt the keys on the server to make them unguessable. But once we added the concept of access tokens for that purpose, this motivation was gone. The remaining motivation was to keep the contract as simple as possible. However, there's a lot of existing code that relies on manipulating the "decoded" form of the segment values. Since we're sending the full segment value anyway, it's wasteful to also send the encoded form, given that it can be trivially re-derived by the client. My immediate motivation is that, I need to be able to construct a cache key from a FlightRouterState. As a bonus, this will also get us closer to being able to remove the dynamic param values from the server response, a separate goal that we're currently working on. We can revisit this once if/when we're able to remove the existing cases where the full segment is needed by the client.

Based on: - #73945 - #73853 - #73667 - #73877 --- This implements support for the Segment Cache in projects/pages where PPR is not enabled. I originally thought the Segment Cache would be tied to the roll out of PPR, because to take full advantage of per-segment caching, you need PPR to generate static shells for each segment. However, as I was investigating how to support the incremental PPR opt-in API, where some pages support PPR and others don't — perhaps just by falling back to the old cache implementation — I realized that there are benefits to per-segment caching even if the requests themselves are not per-segment. For example, when performing a non-PPR-style prefetch, the server diffs the prefetched page against a description of the base page sent by the client. In the old caching implementation, the base tree represented whatever the current page was at the time of the prefetch. In the Segment Cache implementation, we improve on this by telling the server to exclude any shared layouts that are already in the client cache. This ended up requiring more code than I would have preferred, because dynamic server responses and per-segment server responses have differentformats and constraints. However, I realized I was going to have to implement most of this regardless in order to support `<Link prefetch={true}>`, which performs a full dynamic request. Once more of the pieces are settled, we can simplify the implementation by unifying/redesigning some of the data structures, especially FlightRouterState, which has become overloaded with many different concerns. But we need to land some of our experiments first — one reason there's more code than you might expect is because of all the different combinations of features/experiments we need to support simultaneously. While it's likely that PPR will be enabled by default sometime within the next few release cycles, supporting non-PPR projects means we have a pathway to rolling out additional prefetching improvements (like prioritization and cancellation) independently of the PPR release.

ijjk added created-by: Next.js team PRs by the Next.js team. type: next labels Dec 12, 2024

acdlite commented Dec 12, 2024

View reviewed changes

acdlite force-pushed the move-cache-key-creation branch from f36569b to 1176bb1 Compare December 15, 2024 05:56

ijjk added the tests label Dec 15, 2024

acdlite mentioned this pull request Dec 16, 2024

[Segment Cache] Support for non-PPR projects/pages #73960

Merged

acdlite marked this pull request as ready for review December 16, 2024 04:45

acdlite force-pushed the move-cache-key-creation branch from 1176bb1 to 8437186 Compare December 16, 2024 22:22

ztanner approved these changes Dec 16, 2024

View reviewed changes

acdlite force-pushed the move-cache-key-creation branch from 8437186 to b310f42 Compare December 17, 2024 16:38

acdlite force-pushed the move-cache-key-creation branch from b310f42 to 0ea36b0 Compare December 17, 2024 16:40

acdlite enabled auto-merge (squash) December 17, 2024 16:42

acdlite merged commit 6ed03f4 into vercel:canary Dec 17, 2024
103 checks passed

github-actions bot added the locked label Jan 1, 2025

github-actions bot locked as resolved and limited conversation to collaborators Jan 1, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Segment Cache] Move cache key creation to client #73853

[Segment Cache] Move cache key creation to client #73853

acdlite commented Dec 12, 2024 •

edited

Loading

acdlite Dec 12, 2024

acdlite Dec 12, 2024

ijjk commented Dec 12, 2024 •

edited

Loading

ijjk commented Dec 12, 2024 •

edited

Loading

ztanner Dec 16, 2024

acdlite Dec 17, 2024

[Segment Cache] Move cache key creation to client #73853

[Segment Cache] Move cache key creation to client #73853

Conversation

acdlite commented Dec 12, 2024 • edited Loading

acdlite Dec 12, 2024

Choose a reason for hiding this comment

acdlite Dec 12, 2024

Choose a reason for hiding this comment

ijjk commented Dec 12, 2024 • edited Loading

Failing test suites

ijjk commented Dec 12, 2024 • edited Loading

Stats from current PR

ztanner Dec 16, 2024

Choose a reason for hiding this comment

acdlite Dec 17, 2024

Choose a reason for hiding this comment

acdlite commented Dec 12, 2024 •

edited

Loading

ijjk commented Dec 12, 2024 •

edited

Loading

ijjk commented Dec 12, 2024 •

edited

Loading