-
Notifications
You must be signed in to change notification settings - Fork 87
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
fix: update cache handler to accomodate changes in next@canary #2480
Changes from all commits
79b5e39
64def62
073a98e
25ed0e4
c60ac69
01168ef
bb734c0
ff8d9c9
eb14049
2324bd6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,10 +6,12 @@ import { trace } from '@opentelemetry/api' | |
import { wrapTracer } from '@opentelemetry/api/experimental' | ||
import { glob } from 'fast-glob' | ||
import pLimit from 'p-limit' | ||
import { satisfies } from 'semver' | ||
|
||
import { encodeBlobKey } from '../../shared/blobkey.js' | ||
import type { | ||
CachedFetchValue, | ||
NetlifyCachedAppPageValue, | ||
NetlifyCachedPageValue, | ||
NetlifyCachedRouteValue, | ||
NetlifyCacheHandlerValue, | ||
|
@@ -46,13 +48,29 @@ const buildPagesCacheValue = async (path: string): Promise<NetlifyCachedPageValu | |
kind: 'PAGE', | ||
html: await readFile(`${path}.html`, 'utf-8'), | ||
pageData: JSON.parse(await readFile(`${path}.json`, 'utf-8')), | ||
postponed: undefined, | ||
headers: undefined, | ||
status: undefined, | ||
}) | ||
|
||
const buildAppCacheValue = async (path: string): Promise<NetlifyCachedPageValue> => { | ||
const buildAppCacheValue = async ( | ||
path: string, | ||
shouldUseAppPageKind: boolean, | ||
): Promise<NetlifyCachedAppPageValue | NetlifyCachedPageValue> => { | ||
const meta = JSON.parse(await readFile(`${path}.meta`, 'utf-8')) | ||
const html = await readFile(`${path}.html`, 'utf-8') | ||
|
||
// supporting both old and new cache kind for App Router pages - https://github.com/vercel/next.js/pull/65988 | ||
if (shouldUseAppPageKind) { | ||
return { | ||
kind: 'APP_PAGE', | ||
html, | ||
rscData: await readFile(`${path}.rsc`, 'base64').catch(() => | ||
readFile(`${path}.prefetch.rsc`, 'base64'), | ||
), | ||
...meta, | ||
} | ||
} | ||
|
||
const rsc = await readFile(`${path}.rsc`, 'utf-8').catch(() => | ||
readFile(`${path}.prefetch.rsc`, 'utf-8'), | ||
) | ||
|
@@ -66,10 +84,9 @@ const buildAppCacheValue = async (path: string): Promise<NetlifyCachedPageValue> | |
) { | ||
meta.status = 404 | ||
} | ||
|
||
return { | ||
kind: 'PAGE', | ||
html: await readFile(`${path}.html`, 'utf-8'), | ||
html, | ||
pageData: rsc, | ||
...meta, | ||
} | ||
|
@@ -103,6 +120,18 @@ export const copyPrerenderedContent = async (ctx: PluginContext): Promise<void> | |
|
||
const limitConcurrentPrerenderContentHandling = pLimit(10) | ||
|
||
// https://github.com/vercel/next.js/pull/65988 introduced Cache kind specific to pages in App Router (`APP_PAGE`). | ||
// Before this change there was common kind for both Pages router and App router pages | ||
// so we check Next.js version to decide how to generate cache values for App Router pages. | ||
// 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', { | ||
Comment on lines
+126
to
+130
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could pass a mock There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe a better alternative would be to abstract out the version management, so we can just call something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I can still move the check to some better place, but it's only needed once in single place at build time right now |
||
includePrerelease: true, | ||
}) | ||
: false | ||
|
||
await Promise.all( | ||
Object.entries(manifest.routes).map( | ||
([route, meta]): Promise<void> => | ||
|
@@ -125,7 +154,10 @@ export const copyPrerenderedContent = async (ctx: PluginContext): Promise<void> | |
value = await buildPagesCacheValue(join(ctx.publishDir, 'server/pages', key)) | ||
break | ||
case meta.dataRoute?.endsWith('.rsc'): | ||
value = await buildAppCacheValue(join(ctx.publishDir, 'server/app', key)) | ||
value = await buildAppCacheValue( | ||
join(ctx.publishDir, 'server/app', key), | ||
shouldUseAppPageKind, | ||
) | ||
break | ||
case meta.dataRoute === null: | ||
value = await buildRouteCacheValue( | ||
|
@@ -147,7 +179,10 @@ export const copyPrerenderedContent = async (ctx: PluginContext): Promise<void> | |
if (existsSync(join(ctx.publishDir, `server/app/_not-found.html`))) { | ||
const lastModified = Date.now() | ||
const key = '/404' | ||
const value = await buildAppCacheValue(join(ctx.publishDir, 'server/app/_not-found')) | ||
const value = await buildAppCacheValue( | ||
join(ctx.publishDir, 'server/app/_not-found'), | ||
shouldUseAppPageKind, | ||
) | ||
await writeCacheEntry(key, value, lastModified, ctx) | ||
} | ||
} catch (error) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,6 +25,8 @@ export const setRunConfig = (config: NextConfigComplete) => { | |
// set the path to the cache handler | ||
config.experimental = { | ||
...config.experimental, | ||
// @ts-expect-error incrementalCacheHandlerPath was removed from config type | ||
// but we still need to set it for older Next.js versions | ||
incrementalCacheHandlerPath: cacheHandler, | ||
Comment on lines
+28
to
30
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. one of type changes in next canary/rc :( |
||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did this to get updated Cache related types from
next
, otherwise all theAPP_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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we're only using the
next
dependency for types, so I think this approach is okay