fix: update cache handler to accommodate changes in next@canary #2572
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
added
the
test all versions
📊 Package size report 0.03%↑
Unchanged files
🤖 This report was automatically generated by pkg-size-action |
pieh
force-pushed
the
tests-canary-update
branch
from
096a45a to
7da023f
Compare
pieh
commented
Aug 27, 2024
| } else { | ||
| // npm is the default | ||
| cmd = `npm install --no-audit --progress=false --prefer-offline --legacy-peer-deps` | ||
| await rm(join(cwd, 'package-lock.json'), { force: true }) |
There was a problem hiding this comment.
This is for local deving. Doesn't impact our CI runs, because lock files for fixtures are not commited
pieh
changed the title
pieh
commented
Aug 27, 2024
pieh
commented
Aug 27, 2024
Comment on lines
+100
to
+105
| const { stdout } = await execaCommand( | ||
| `npm info next@${resolvedVersion} peerDependencies --json`, | ||
| { cwd }, | ||
| ) | ||
|
|
||
| const nextPeerDependencies = JSON.parse(stdout) |
There was a problem hiding this comment.
next@canary started to bump react(-dom) peer dependencies quite frequently, so instead of hardcoding react version - we just check what react version given next version needs
pieh
commented
Aug 28, 2024
Comment on lines
+69
to
+71
| export type CachedFetchValueForMultipleVersions = Omit<CachedFetchValue, 'kind'> & { | ||
| kind: 'FETCH' | ||
| } |
There was a problem hiding this comment.
This one is really just to satisfy TS. Next now uses const enum with string values and you can't assign string literal to it (even tho the value matches).
For other similar cases like this - i.e. kind: 'PAGE' | 'PAGES' or kind: 'ROUTE' | 'APP_ROUTE' - Next actually changed the string value from PAGE to PAGES and ROUTE to APP_ROUTE and we want to support both.
For our CacheHandler implementation we do receive appropriate kind used by Next.js in .set variant as it's Next.js that produce cache value to store.
For .get case we check what we stored in handle things appropriately for kinds before and after kind change and we don't need to make any decision which should be used there.
We do have to figure out which kind to actually use when seeding blobs from prerendered content, so that's the only case that need to decide which kind to use (and it's done based on Next.js version)
serhalp
previously approved these changes
Aug 28, 2024
| @@ -97,8 +96,17 @@ export async function setNextVersionInFixture( | |||
| return | |||
| } | |||
| packageJson.dependencies.next = version | |||
|
|
|||
| const { stdout } = await execaCommand( | |||
| `npm info next@${resolvedVersion} peerDependencies --json`, | |||
There was a problem hiding this comment.
I'm just curious why 🙈 on this one. I more than agree on all other cases of 🙈 ... "notes" :) But this one I think is reasonable way to solve the react(-dom) peerDeps problem with next@canary? Unless this is more about the need to do something like this than doing what I'm doing here?
There was a problem hiding this comment.
Yes it's about the need to do this at all, not about the solution you went with! :)
| @@ -133,6 +139,13 @@ export const copyPrerenderedContent = async (ctx: PluginContext): Promise<void> | |||
| }) | |||
| : false | |||
|
|
|||
| // https://github.com/vercel/next.js/pull/68602 changed the cache kind for Pages router pages from `PAGE` to `PAGES` and from `ROUTE` to `APP_ROUTE`. | |||
| const shouldUseEnumKind = ctx.nextVersion | |||
| ? satisfies(ctx.nextVersion, '>=15.0.0-canary.114 <15.0.0-d || >15.0.0-rc.0', { | |||
Comment on lines
+94
to
+106
| type IncrementalCacheValueToMultipleVersions<T> = T extends CachedRouteValue | ||
| ? CachedRouteValueForMultipleVersions | ||
| : T extends IncrementalCachedPageValue | ||
| ? IncrementalCachedPageValueForMultipleVersions | ||
| : T extends IncrementalCachedAppPageValue | ||
| ? IncrementalCachedAppPageValueForMultipleVersions | ||
| : T extends CachedFetchValue | ||
| ? CachedFetchValueForMultipleVersions | ||
| : T extends CacheHandlerValue | ||
| ? { | ||
| [K in keyof T]: IncrementalCacheValueToMultipleVersions<T[K]> | ||
| } | ||
| : T |
There was a problem hiding this comment.
Yeah, this whole thing is quite a mess. It doesn't help that there are 2 type variants generally created there:
- (new)mapping types from currently used
nextversion we have for types to not that version specific but instead have kind of union that "work" for all the next versions we support (which is what this inline comment highlights) - Mapping those types to how we actually store data in blobs (i.e. instead of using
Bufferwe usestringfor storage or adding extra information aboutrevalidatethat is used to figure out cacheable route handler response maxage).
For 1. I did try having multiple next version in deps and import types from there and have some kind of union on them, but setup for it was pretty messy:
- I could not use npm alias and have multiple next versions there because their react(-dom) peerDeps could not be satisfied at the same time)
- Using unions there was still not enough because next@canary now uses const enum so I would have to do messy things either way.
So I don't think we should treat those types necessarily as safeguard that we are using them correctly for Next purposes (unfortunately :( ).
The option where this could maybe work is if we would have actual 2 (or more) variants of CacheHandler implementations, but because those would be largely the same we would either have almost copies of them or maybe some abstraction that would be more difficult to read :/
Co-authored-by: Philippe Serhal <philippe.serhal@netlify.com>
orinokai
approved these changes
Aug 29, 2024
There was a problem hiding this comment.
LGTM and 👏🏼 for dealing with this all. It's also made me think we should discuss our options regarding long-term support of different Next versions. I feel like there might be trade-offs we can make that could avoid incrementally adding complexity like this. Will start a doc...
# for free
to join this conversation on GitHub.
Already have an account?
# 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
Adjusting for recent changes in
next@canary:kindchanges (PAGE->PAGESandROUTE->APP_ROUTE) - see: refactor: incremental cache cleanup vercel/next.js#68602 (not the most immediately visible, but https://github.com/vercel/next.js/pull/68602/files#diff-cdee3ad3788ca17ac8dbaa342fe7e7ac4b1362cbc10b7bf22f7568302c995584R40-R47 is current enum for it)NODE_ENV=productionenv var - https://github.com/vercel/next.js/pull/68193/files#diff-37243d614f1f5d3f7ea50bbf2af263f6b1a9a4f70e84427977781e07b02f57f1R49 this import indirectly requiresreactand at this point nothing setsNODE_ENVyet which results in loading development react version (which is later used forever due to being stored inrequire.cache). This later causes some conflicts as actual rendering will use production react version. Do note thatNODE_ENVsetting is being done in standalone output entry point that Next.js produces - https://github.com/vercel/next.js/blob/c3c38bd191ae0343cb1a449b00970ede0913ea8d/packages/next/src/build/utils.ts#L2221 but we aren't actually using this entrypoint, so setting it ourselves brings as closer to Next.js overall while also fixing dev/prod react problem)Documentation
Tests
You can test this change yourself like so:
Relevant links (GitHub issues, etc.) or a picture of cute animal