fix: update cache handler to accomodate changes in next@canary #2480
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pieh
changed the title
…d content for APP_PAGE
…ored, because it no longer exist in newest major, but we still have to set it for older next versions
📊 Package size report 0.04%↑
Unchanged files
🤖 This report was automatically generated by pkg-size-action |
pieh
force-pushed
the
fix/next-canary-shared-revalidate-timings-app-page
branch
from
f8b8d9c
to
c60ac69
Compare
pieh
added
the
test all versions
pieh
commented
Jun 14, 2024
| @@ -79,7 +79,7 @@ | |||
| "memfs": "^4.9.2", | |||
| "mock-require": "^3.0.3", | |||
| "msw": "^2.0.7", | |||
| "next": "^14.0.4", | |||
| "next": "^15.0.0-canary.28", | |||
There was a problem hiding this comment.
I did this to get updated Cache related types from next, otherwise all the APP_PAGE related additions were causing a lot of TS issues.
I'm not sure if doing this is the best or wether I should maybe copy over relevant types until those become available in latest next?
There was a problem hiding this comment.
I think we're only using the next dependency for types, so I think this approach is okay
pieh
commented
Jun 14, 2024
Comment on lines
+28
to
30
| // @ts-expect-error incrementalCacheHandlerPath was removed from config type | ||
| // but we still need to set it for older Next.js versions | ||
| incrementalCacheHandlerPath: cacheHandler, |
There was a problem hiding this comment.
one of type changes in next canary/rc :(
pieh
force-pushed
the
fix/next-canary-shared-revalidate-timings-app-page
branch
from
2b4eb38
to
7da721e
Compare
…version after build command finished
pieh
force-pushed
the
fix/next-canary-shared-revalidate-timings-app-page
branch
from
7da721e
to
01168ef
Compare
pieh
force-pushed
the
fix/next-canary-shared-revalidate-timings-app-page
branch
from
1eb088f
to
bb734c0
Compare
pieh
commented
Jun 14, 2024
Comment on lines
+126
to
+130
| // Note: at time of writing this code, released 15@rc uses old kind for App Router pages, while 15.0.0@canary.13 and newer canaries use new kind. | ||
| // Looking at 15@rc release branch it was merging `canary` branch in, so the version constraint assumes that future 15@rc (and 15@latest) versions | ||
| // will use new kind for App Router pages. | ||
| const shouldUseAppPageKind = ctx.nextVersion | ||
| ? satisfies(ctx.nextVersion, '>=15.0.0-canary.13 <15.0.0-d || >15.0.0-rc.0', { |
There was a problem hiding this comment.
This is a bit tricky as it tries to predict what future rc/stable releases of Next would likely contain and this is my best guess in hope that version released with this code would be usable with future next versions.
The alternative I was wondering about is doing a test by checking some internal modules, but the Next.js change only added some helper exports ( i.e. https://github.com/vercel/next.js/pull/65988/files#diff-17519b0578e6b6493027a0faa49f2aaab1a70d81216339a93a8d87c68a861f0d ) and only clear APP_PAGE things were types which I can't check in runtime :/ But also this test would have to maintained into the future, so this overall is not that great of an idea (IMO)
There was a problem hiding this comment.
I agree with you about this being difficult to maintain. I did a bit of spelunking and found this util that we could potentially use to ascertain whether APP_PAGE is supported https://github.com/vercel/next.js/blob/658037358cf4c9813648e3a9b1c70d7e414c87ff/packages/next/src/server/response-cache/utils.ts#L5
There was a problem hiding this comment.
How would we use that to test it? This helper was added long ago so it couldn't be just testing if there is export like that?
There was a problem hiding this comment.
We could pass a mock ResponseCacheEntry object to it with value.kind = 'APP_PAGE' and value.rscData set. If APP_PAGE is supported it will return an IncrementalCacheItem with value.rscData still set (canary behaviour) and if not it will return a IncrementalCacheItem without value.rscData (stable behaviour).
There was a problem hiding this comment.
Ok, that makes sense, but do we want to introduce test like (relying on specific behavior of Next.js internal helper) that that we will need to possibly maintain (including finding alternatives if it ever changes)? It feels like more work over-time than possibly adjusting version range above if expectation of the change shipping with -rc.1 will be incorrect?
There was a problem hiding this comment.
Maybe a better alternative would be to abstract out the version management, so we can just call something like hasAppPage() in the cache handler (and subsequent other parts of the code) and the abstraction takes care of the version detection and mapping to version numbers. Then we just accept that we'll need to monitor versions and make changes, but at least it would just be in one place? This could be a follow up PR, so I'm approving this one.
There was a problem hiding this comment.
CacheHandler doesn't need to have such check. The PAGE kind is the same so all the handling for that remain and APP_PAGE kind has separate handling. The only place that needs conditional logic like that would be when we prepare cache entries from prerendered content when deciding which kind the content should be and where to put rsc data in the cache entry + wether it should be plain text or encoded content.
I can still move the check to some better place, but it's only needed once in single place at build time right now
orinokai
approved these changes
Jun 18, 2024
Comment on lines
+126
to
+130
| // Note: at time of writing this code, released 15@rc uses old kind for App Router pages, while 15.0.0@canary.13 and newer canaries use new kind. | ||
| // Looking at 15@rc release branch it was merging `canary` branch in, so the version constraint assumes that future 15@rc (and 15@latest) versions | ||
| // will use new kind for App Router pages. | ||
| const shouldUseAppPageKind = ctx.nextVersion | ||
| ? satisfies(ctx.nextVersion, '>=15.0.0-canary.13 <15.0.0-d || >15.0.0-rc.0', { |
There was a problem hiding this comment.
Maybe a better alternative would be to abstract out the version management, so we can just call something like hasAppPage() in the cache handler (and subsequent other parts of the code) and the abstraction takes care of the version detection and mapping to version numbers. Then we just accept that we'll need to monitor versions and make changes, but at least it would just be in one place? This could be a follow up PR, so I'm approving this one.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
There were quite a bit of changes in
next@canarythat we currently don't handle:APP_PAGE- Re-land Fix broken HTML inlining of non UTF-8 decodable binary data from Flight payload #65664 vercel/next.js#65988This one means we need to update both our CacheHandler to support that kind (should be done) and our build time handling of prerendered content (not fully done yet - missing part is correct condition to use current or new one cache kind - right now I'm relying on our test env var to at least test that blob shape is correct)
PrerenderManifestwhen doing staleness calculations. Instead they now useSharedRevalidateTimingsfor that ( Shared Revalidate Timings vercel/next.js#64370 ) and madePrerenderManifestfrozen ( Freeze loaded manifests vercel/next.js#64313 ) which means we can't mutate it and so c91e257 is causing actually more problems there (I'm also trying to add some graceful error handling with addition of try/catches so in case the hacks that we have stop working it's not completely borked)Documentation
Tests
You can test this change yourself like so:
Relevant links (GitHub issues, etc.) or a picture of cute animal