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

fix: update cache handler to accomodate changes in next@canary #2480

Merged
merged 10 commits into from
Jun 18, 2024
Merged
1,419 changes: 1,081 additions & 338 deletions package-lock.json

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Copy link
Contributor Author

@pieh pieh Jun 14, 2024

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 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?

Copy link
Contributor

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

"os": "^0.1.2",
"outdent": "^0.8.0",
"p-limit": "^5.0.0",
Expand Down
47 changes: 41 additions & 6 deletions src/build/content/prerendered.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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'),
)
Expand All @@ -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,
}
Expand Down Expand Up @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 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)

Copy link
Contributor

Choose a reason for hiding this comment

The 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 APP_PAGE is supported https://github.com/vercel/next.js/blob/658037358cf4c9813648e3a9b1c70d7e414c87ff/packages/next/src/server/response-cache/utils.ts#L5

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

@orinokai orinokai Jun 17, 2024

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 -rc.1 will be incorrect?

Copy link
Contributor

@orinokai orinokai Jun 18, 2024

Choose a reason for hiding this comment

The 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 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.

Copy link
Contributor Author

@pieh pieh Jun 18, 2024

Choose a reason for hiding this comment

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

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

includePrerelease: true,
})
: false

await Promise.all(
Object.entries(manifest.routes).map(
([route, meta]): Promise<void> =>
Expand All @@ -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(
Expand All @@ -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) {
Expand Down
20 changes: 3 additions & 17 deletions src/build/content/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import { prerelease, lt as semverLowerThan, lte as semverLowerThanOrEqual } from

import { RUN_CONFIG } from '../../run/constants.js'
import { PluginContext } from '../plugin-context.js'
import { verifyNextVersion } from '../verification.js'

const tracer = wrapTracer(trace.getTracer('Next runtime'))

Expand Down Expand Up @@ -292,26 +291,13 @@ export const copyNextDependencies = async (ctx: PluginContext): Promise<void> =>

await Promise.all(promises)

// detect if it might lead to a runtime issue and throw an error upfront on build time instead of silently failing during runtime
const serverHandlerRequire = createRequire(posixJoin(ctx.serverHandlerDir, ':internal:'))

let nextVersion: string | undefined
try {
const { version } = serverHandlerRequire('next/package.json')
if (version) {
nextVersion = version as string
}
} catch {
// failed to resolve package.json - currently this is resolvable in all known next versions, but if next implements
// exports map it still might be a problem in the future, so we are not breaking here
}

if (nextVersion) {
verifyNextVersion(ctx, nextVersion)

await patchNextModules(ctx, nextVersion, serverHandlerRequire.resolve)
if (ctx.nextVersion) {
await patchNextModules(ctx, ctx.nextVersion, serverHandlerRequire.resolve)
}

// detect if it might lead to a runtime issue and throw an error upfront on build time instead of silently failing during runtime
try {
const nextEntryAbsolutePath = serverHandlerRequire.resolve('next')
const nextRequire = createRequire(nextEntryAbsolutePath)
Expand Down
21 changes: 21 additions & 0 deletions src/build/plugin-context.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import { existsSync, readFileSync } from 'node:fs'
import { readFile } from 'node:fs/promises'
import { createRequire } from 'node:module'
import { join, relative, resolve } from 'node:path'
import { join as posixJoin } from 'node:path/posix'
import { fileURLToPath } from 'node:url'

import type {
Expand Down Expand Up @@ -313,6 +315,25 @@ export class PluginContext {
return JSON.parse(await readFile(join(this.publishDir, 'routes-manifest.json'), 'utf-8'))
}

#nextVersion: string | null | undefined = undefined

/**
* Get Next.js version that was used to build the site
*/
get nextVersion(): string | null {
if (this.#nextVersion === undefined) {
try {
const serverHandlerRequire = createRequire(posixJoin(this.standaloneRootDir, ':internal:'))
const { version } = serverHandlerRequire('next/package.json')
this.#nextVersion = version as string
} catch {
this.#nextVersion = null
}
}

return this.#nextVersion
}

/** Fails a build with a message and an optional error */
failBuild(message: string, error?: unknown): never {
return this.utils.build.failBuild(message, error instanceof Error ? { error } : undefined)
Expand Down
17 changes: 9 additions & 8 deletions src/build/verification.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,15 @@ export function verifyPublishDir(ctx: PluginContext) {
`Your publish directory does not contain expected Next.js build output. Please make sure you are using Next.js version (${SUPPORTED_NEXT_VERSIONS})`,
)
}

if (
ctx.nextVersion &&
!satisfies(ctx.nextVersion, SUPPORTED_NEXT_VERSIONS, { includePrerelease: true })
) {
ctx.failBuild(
`@netlify/plugin-next@5 requires Next.js version ${SUPPORTED_NEXT_VERSIONS}, but found ${ctx.nextVersion}. Please upgrade your project's Next.js version.`,
)
}
}
if (ctx.buildConfig.output === 'export') {
if (!ctx.exportDetail?.success) {
Expand All @@ -60,14 +69,6 @@ export function verifyPublishDir(ctx: PluginContext) {
}
}

export function verifyNextVersion(ctx: PluginContext, nextVersion: string): void | never {
if (!satisfies(nextVersion, SUPPORTED_NEXT_VERSIONS, { includePrerelease: true })) {
ctx.failBuild(
`@netlify/plugin-next@5 requires Next.js version ${SUPPORTED_NEXT_VERSIONS}, but found ${nextVersion}. Please upgrade your project's Next.js version.`,
)
}
}

export async function verifyNoAdvancedAPIRoutes(ctx: PluginContext) {
const apiRoutesConfigs = await getAPIRoutesConfigs(ctx)

Expand Down
2 changes: 2 additions & 0 deletions src/run/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

one of type changes in next canary/rc :(

}

Expand Down
79 changes: 61 additions & 18 deletions src/run/handlers/cache.cts
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,6 @@ import { purgeCache } from '@netlify/functions'
import { type Span } from '@opentelemetry/api'
import type { PrerenderManifest } from 'next/dist/build/index.js'
import { NEXT_CACHE_TAGS_HEADER } from 'next/dist/lib/constants.js'
import { loadManifest } from 'next/dist/server/load-manifest.js'
import { normalizePagePath } from 'next/dist/shared/lib/page-path/normalize-page-path.js'

import type {
CacheHandler,
Expand Down Expand Up @@ -111,23 +109,41 @@ export class NetlifyCacheHandler implements CacheHandler {
return restOfRouteValue
}

private injectEntryToPrerenderManifest(
private async injectEntryToPrerenderManifest(
key: string,
revalidate: NetlifyCachedPageValue['revalidate'],
) {
if (this.options.serverDistDir && (typeof revalidate === 'number' || revalidate === false)) {
const prerenderManifest = loadManifest(
join(this.options.serverDistDir, '..', 'prerender-manifest.json'),
) as PrerenderManifest

prerenderManifest.routes[key] = {
experimentalPPR: undefined,
dataRoute: posixJoin('/_next/data', `${normalizePagePath(key)}.json`),
srcRoute: null, // FIXME: provide actual source route, however, when dynamically appending it doesn't really matter
initialRevalidateSeconds: revalidate,
// Pages routes do not have a prefetch data route.
prefetchDataRoute: undefined,
}
try {
const { loadManifest } = await import('next/dist/server/load-manifest.js')
const prerenderManifest = loadManifest(
join(this.options.serverDistDir, '..', 'prerender-manifest.json'),
) as PrerenderManifest

try {
const { normalizePagePath } = await import(
'next/dist/shared/lib/page-path/normalize-page-path.js'
)

prerenderManifest.routes[key] = {
experimentalPPR: undefined,
dataRoute: posixJoin('/_next/data', `${normalizePagePath(key)}.json`),
srcRoute: null, // FIXME: provide actual source route, however, when dynamically appending it doesn't really matter
initialRevalidateSeconds: revalidate,
// Pages routes do not have a prefetch data route.
prefetchDataRoute: undefined,
}
} catch {
// depending on Next.js version - prerender manifest might not be mutable
// https://github.com/vercel/next.js/pull/64313
// if it's not mutable we will try to use SharedRevalidateTimings ( https://github.com/vercel/next.js/pull/64370) instead
const { SharedRevalidateTimings } = await import(
'next/dist/server/lib/incremental-cache/shared-revalidate-timings.js'
)
const sharedRevalidateTimings = new SharedRevalidateTimings(prerenderManifest)
sharedRevalidateTimings.set(key, revalidate)
}
} catch {}
}
}

Expand Down Expand Up @@ -178,7 +194,7 @@ export class NetlifyCacheHandler implements CacheHandler {
lastModified: blob.lastModified,
value: {
...valueWithoutRevalidate,
body: Buffer.from(valueWithoutRevalidate.body as unknown as string, 'base64'),
body: Buffer.from(valueWithoutRevalidate.body, 'base64'),
},
}
}
Expand All @@ -187,13 +203,28 @@ export class NetlifyCacheHandler implements CacheHandler {

const { revalidate, ...restOfPageValue } = blob.value

this.injectEntryToPrerenderManifest(key, revalidate)
await this.injectEntryToPrerenderManifest(key, revalidate)

return {
lastModified: blob.lastModified,
value: restOfPageValue,
}
}
case 'APP_PAGE': {
span.addEvent('APP_PAGE', { lastModified: blob.lastModified })

const { revalidate, rscData, ...restOfPageValue } = blob.value

await this.injectEntryToPrerenderManifest(key, revalidate)

return {
lastModified: blob.lastModified,
value: {
...restOfPageValue,
rscData: rscData ? Buffer.from(rscData, 'base64') : undefined,
},
}
}
default:
span.recordException(new Error(`Unknown cache entry kind: ${blob.value?.kind}`))
}
Expand All @@ -220,6 +251,14 @@ export class NetlifyCacheHandler implements CacheHandler {
}
}

if (data?.kind === 'APP_PAGE') {
return {
...data,
revalidate: context.revalidate,
rscData: data.rscData?.toString('base64'),
}
}

return data
}

Expand Down Expand Up @@ -299,7 +338,11 @@ export class NetlifyCacheHandler implements CacheHandler {

if (cacheEntry.value?.kind === 'FETCH') {
cacheTags = [...tags, ...softTags]
} else if (cacheEntry.value?.kind === 'PAGE' || cacheEntry.value?.kind === 'ROUTE') {
} else if (
cacheEntry.value?.kind === 'PAGE' ||
cacheEntry.value?.kind === 'APP_PAGE' ||
cacheEntry.value?.kind === 'ROUTE'
) {
cacheTags = (cacheEntry.value.headers?.[NEXT_CACHE_TAGS_HEADER] as string)?.split(',') || []
} else {
return false
Expand Down
18 changes: 16 additions & 2 deletions src/shared/cache-types.cts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import type {
} from 'next/dist/server/lib/incremental-cache/index.js'
import type {
CachedRouteValue,
IncrementalCachedAppPageValue,
IncrementalCacheValue,
} from 'next/dist/server/response-cache/types.js'

Expand All @@ -21,6 +22,12 @@ export type NetlifyCachedRouteValue = Omit<CachedRouteValue, 'body'> & {
revalidate: Parameters<IncrementalCache['set']>[2]['revalidate']
}

export type NetlifyCachedAppPageValue = Omit<IncrementalCachedAppPageValue, 'rscData'> & {
// Next.js stores rscData as buffer, while we store it as base64 encoded string
rscData: string | undefined
revalidate?: Parameters<IncrementalCache['set']>[2]['revalidate']
}

type CachedPageValue = Extract<IncrementalCacheValue, { kind: 'PAGE' }>

export type NetlifyCachedPageValue = CachedPageValue & {
Expand All @@ -30,15 +37,22 @@ export type NetlifyCachedPageValue = CachedPageValue & {
export type CachedFetchValue = Extract<IncrementalCacheValue, { kind: 'FETCH' }>

export type NetlifyIncrementalCacheValue =
| Exclude<IncrementalCacheValue, CachedRouteValue | CachedPageValue>
| Exclude<
IncrementalCacheValue,
CachedRouteValue | CachedPageValue | IncrementalCachedAppPageValue
>
| NetlifyCachedRouteValue
| NetlifyCachedPageValue
| NetlifyCachedAppPageValue

type CachedRouteValueToNetlify<T> = T extends CachedRouteValue
? NetlifyCachedRouteValue
: T extends CachedPageValue
? NetlifyCachedPageValue
: T
: T extends IncrementalCachedAppPageValue
? NetlifyCachedAppPageValue
: T

type MapCachedRouteValueToNetlify<T> = { [K in keyof T]: CachedRouteValueToNetlify<T[K]> }

export type NetlifyCacheHandlerValue = MapCachedRouteValueToNetlify<CacheHandlerValue>
Loading
Loading