Skip to content

fix: update cache handler to accommodate changes in next@canary #2572

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

Merged
merged 17 commits into from
Aug 29, 2024
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 19 additions & 5 deletions src/build/content/prerendered.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,11 @@
*/
const routeToFilePath = (path: string) => (path === '/' ? '/index' : path)

const buildPagesCacheValue = async (path: string): Promise<NetlifyCachedPageValue> => ({
kind: 'PAGE',
const buildPagesCacheValue = async (
path: string,
shouldUseEnumKind: boolean,
): Promise<NetlifyCachedPageValue> => ({

Check failure on line 51 in src/build/content/prerendered.ts

View workflow job for this annotation

GitHub Actions / Lint

Type '{ kind: string; html: string; pageData: any; headers: undefined; status: undefined; }' is not assignable to type 'never'.
kind: shouldUseEnumKind ? 'PAGES' : 'PAGE',
html: await readFile(`${path}.html`, 'utf-8'),
pageData: JSON.parse(await readFile(`${path}.json`, 'utf-8')),
headers: undefined,
Expand Down Expand Up @@ -96,8 +99,9 @@
const buildRouteCacheValue = async (
path: string,
initialRevalidateSeconds: number | false,
shouldUseEnumKind: boolean,
): Promise<NetlifyCachedRouteValue> => ({
kind: 'ROUTE',
kind: shouldUseEnumKind ? 'APP_ROUTE' : 'ROUTE',
body: await readFile(`${path}.body`, 'base64'),
...JSON.parse(await readFile(`${path}.meta`, 'utf-8')),
revalidate: initialRevalidateSeconds,
Expand Down Expand Up @@ -133,6 +137,12 @@
})
: false

const shouldUseEnumKind = ctx.nextVersion
? satisfies(ctx.nextVersion, '>=15.0.0-canary.114 <15.0.0-d || >15.0.0-rc.0', {
Copy link
Contributor

Choose a reason for hiding this comment

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

🙈

includePrerelease: true,
})
: false

await Promise.all(
Object.entries(manifest.routes).map(
([route, meta]): Promise<void> =>
Expand All @@ -152,7 +162,10 @@
// if pages router returns 'notFound: true', build won't produce html and json files
return
}
value = await buildPagesCacheValue(join(ctx.publishDir, 'server/pages', key))
value = await buildPagesCacheValue(
join(ctx.publishDir, 'server/pages', key),
shouldUseEnumKind,
)
break
case meta.dataRoute?.endsWith('.rsc'):
value = await buildAppCacheValue(
Expand All @@ -164,14 +177,15 @@
value = await buildRouteCacheValue(
join(ctx.publishDir, 'server/app', key),
meta.initialRevalidateSeconds,
shouldUseEnumKind,
)
break
default:
throw new Error(`Unrecognized content: ${route}`)
}

// Netlify Forms are not support and require a workaround
if (value.kind === 'PAGE' || value.kind === 'APP_PAGE') {
if (value.kind === 'PAGE' || value.kind === 'PAGES' || value.kind === 'APP_PAGE') {

Check failure on line 188 in src/build/content/prerendered.ts

View workflow job for this annotation

GitHub Actions / Lint

This comparison appears to be unintentional because the types '"ROUTE" | "APP_PAGE"' and '"PAGE"' have no overlap.

Check failure on line 188 in src/build/content/prerendered.ts

View workflow job for this annotation

GitHub Actions / Lint

This comparison appears to be unintentional because the types '"ROUTE" | "APP_PAGE"' and '"PAGES"' have no overlap.
verifyNetlifyForms(ctx, value.html)
}

Expand Down
28 changes: 19 additions & 9 deletions src/run/handlers/cache.cts
Original file line number Diff line number Diff line change
Expand Up @@ -132,13 +132,18 @@

if (
cacheValue.kind === 'PAGE' ||
cacheValue.kind === 'PAGES' ||

Check failure on line 135 in src/run/handlers/cache.cts

View workflow job for this annotation

GitHub Actions / Lint

This comparison appears to be unintentional because the types '"ROUTE" | "REDIRECT" | "APP_PAGE" | "IMAGE" | "FETCH"' and '"PAGES"' have no overlap.
cacheValue.kind === 'APP_PAGE' ||
cacheValue.kind === 'ROUTE'
cacheValue.kind === 'ROUTE' ||
cacheValue.kind === 'APP_ROUTE'

Check failure on line 138 in src/run/handlers/cache.cts

View workflow job for this annotation

GitHub Actions / Lint

This comparison appears to be unintentional because the types '"REDIRECT" | "IMAGE" | "FETCH"' and '"APP_ROUTE"' have no overlap.
) {
if (cacheValue.headers?.[NEXT_CACHE_TAGS_HEADER]) {
const cacheTags = (cacheValue.headers[NEXT_CACHE_TAGS_HEADER] as string).split(',')
requestContext.responseCacheTags = cacheTags
} else if (cacheValue.kind === 'PAGE' && typeof cacheValue.pageData === 'object') {
} else if (
(cacheValue.kind === 'PAGE' || cacheValue.kind === 'PAGES') &&

Check failure on line 144 in src/run/handlers/cache.cts

View workflow job for this annotation

GitHub Actions / Lint

This comparison appears to be unintentional because the types '"ROUTE" | "APP_PAGE"' and '"PAGES"' have no overlap.
typeof cacheValue.pageData === 'object'
) {
// pages router doesn't have cache tags headers in PAGE cache value
// so we need to generate appropriate cache tags for it
const cacheTags = [`_N_T_${key === '/index' ? '/' : key}`]
Expand Down Expand Up @@ -186,6 +191,7 @@
}

async get(...args: Parameters<CacheHandler['get']>): ReturnType<CacheHandler['get']> {
debugger

Check failure on line 194 in src/run/handlers/cache.cts

View workflow job for this annotation

GitHub Actions / Lint

Unexpected 'debugger' statement.
return this.tracer.withActiveSpan('get cache key', async (span) => {
const [key, ctx = {}] = args
getLogger().debug(`[NetlifyCacheHandler.get]: ${key}`)
Expand Down Expand Up @@ -224,8 +230,9 @@
value: blob.value,
}

case 'ROUTE': {
span.addEvent('ROUTE', { lastModified: blob.lastModified, status: blob.value.status })
case 'ROUTE':
case 'APP_ROUTE': {

Check failure on line 234 in src/run/handlers/cache.cts

View workflow job for this annotation

GitHub Actions / Lint

Type '"APP_ROUTE"' is not comparable to type '"ROUTE" | "REDIRECT" | "PAGE" | "APP_PAGE" | "IMAGE" | "FETCH" | undefined'.
span.addEvent('APP_ROUTE', { lastModified: blob.lastModified, status: blob.value.status })

const valueWithoutRevalidate = this.captureRouteRevalidateAndRemoveFromObject(blob.value)

Expand All @@ -237,12 +244,13 @@
},
}
}
case 'PAGE': {
case 'PAGE':
case 'PAGES': {

Check failure on line 248 in src/run/handlers/cache.cts

View workflow job for this annotation

GitHub Actions / Lint

Type '"PAGES"' is not comparable to type '"ROUTE" | "REDIRECT" | "PAGE" | "APP_PAGE" | "IMAGE" | "FETCH" | undefined'.
span.addEvent('PAGE', { lastModified: blob.lastModified })

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

Check failure on line 251 in src/run/handlers/cache.cts

View workflow job for this annotation

GitHub Actions / Lint

Property 'revalidate' does not exist on type 'IncrementalCachedPageValue'.

await this.injectEntryToPrerenderManifest(key, revalidate)

Check failure on line 253 in src/run/handlers/cache.cts

View workflow job for this annotation

GitHub Actions / Lint

Argument of type 'any' is not assignable to parameter of type 'never'.

return {
lastModified: blob.lastModified,
Expand Down Expand Up @@ -275,15 +283,15 @@
data: Parameters<IncrementalCache['set']>[1],
context: Parameters<IncrementalCache['set']>[2],
): NetlifyIncrementalCacheValue | null {
if (data?.kind === 'ROUTE') {
if (data?.kind === 'ROUTE' || data?.kind === 'APP_ROUTE') {
return {
...data,
revalidate: context.revalidate,
body: data.body.toString('base64'),
}
}

if (data?.kind === 'PAGE') {
if (data?.kind === 'PAGE' || data?.kind === 'PAGES') {
return {
...data,
revalidate: context.revalidate,
Expand Down Expand Up @@ -321,7 +329,7 @@
value,
})

if (data?.kind === 'PAGE') {
if (data?.kind === 'PAGE' || data?.kind === 'PAGES') {
const requestContext = getRequestContext()
if (requestContext?.didPagesRouterOnDemandRevalidate) {
const tag = `_N_T_${key === '/index' ? '/' : key}`
Expand Down Expand Up @@ -397,8 +405,10 @@
cacheTags = [...tags, ...softTags]
} else if (
cacheEntry.value?.kind === 'PAGE' ||
cacheEntry.value?.kind === 'PAGES' ||
cacheEntry.value?.kind === 'APP_PAGE' ||
cacheEntry.value?.kind === 'ROUTE'
cacheEntry.value?.kind === 'ROUTE' ||
cacheEntry.value?.kind === 'APP_ROUTE'
) {
cacheTags = (cacheEntry.value.headers?.[NEXT_CACHE_TAGS_HEADER] as string)?.split(',') || []
} else {
Expand Down
3 changes: 3 additions & 0 deletions src/run/handlers/request-context.cts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@ import { LogLevel, systemLogger } from '@netlify/functions/internal'

import type { NetlifyCachedRouteValue } from '../../shared/cache-types.cjs'

// hacks?
process.env.NODE_ENV = 'production'

type SystemLogger = typeof systemLogger

export type RequestContext = {
Expand Down
2 changes: 1 addition & 1 deletion src/shared/cache-types.cts
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ export type NetlifyCachedAppPageValue = Omit<IncrementalCachedAppPageValue, 'rsc
revalidate?: Parameters<IncrementalCache['set']>[2]['revalidate']
}

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

export type NetlifyCachedPageValue = CachedPageValue & {
revalidate?: Parameters<IncrementalCache['set']>[2]['revalidate']
Expand Down
7 changes: 5 additions & 2 deletions tests/prepare.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -44,14 +44,17 @@ const promises = fixtures.map((fixture) =>
})
}

// npm is the default
let cmd = `npm install --no-audit --progress=false --prefer-offline --legacy-peer-deps`
let cmd = ``
const { packageManager } = JSON.parse(await readFile(join(cwd, 'package.json'), 'utf8'))
if (packageManager?.startsWith('pnpm')) {
// We disable frozen-lockfile because we may have changed the version of Next.js
cmd = `pnpm install --frozen-lockfile=false ${
process.env.DEBUG || NEXT_VERSION !== 'latest' ? '' : '--reporter silent'
}`
} 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 })
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 for local deving. Doesn't impact our CI runs, because lock files for fixtures are not commited

}

const addPrefix = new Transform({
Expand Down
1 change: 1 addition & 0 deletions tests/utils/fixture.ts
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ export const createFixture = async (fixture: string, ctx: FixtureTestContext) =>
) {
// @ts-ignore fetch doesn't have _nextOriginalFetch property in types
globalThis.fetch = globalThis._nextOriginalFetch || globalThis.fetch._nextOriginalFetch
delete globalThis[Symbol.for('next-patch')]
}

ctx.cwd = await mkdtemp(join(tmpdir(), 'netlify-next-runtime-'))
Expand Down
14 changes: 11 additions & 3 deletions tests/utils/next-version-helpers.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,11 @@ import { readFile, writeFile } from 'node:fs/promises'

import fg from 'fast-glob'
import { gte, satisfies, valid } from 'semver'
import { execaCommand } from 'execa'

const FUTURE_NEXT_PATCH_VERSION = '14.999.0'

const NEXT_VERSION_REQUIRES_REACT_19 = '14.3.0-canary.45'
// TODO: Update this when React 19 is released
const REACT_19_VERSION = '19.0.0-rc.0'
const REACT_18_VERSION = '18.2.0'

/**
Expand Down Expand Up @@ -97,8 +96,17 @@ export async function setNextVersionInFixture(
return
}
packageJson.dependencies.next = version

const { stdout } = await execaCommand(
`npm info next@${resolvedVersion} peerDependencies --json`,
Copy link
Contributor

Choose a reason for hiding this comment

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

🙈

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes it's about the need to do this at all, not about the solution you went with! :)

{ cwd },
)

const nextPeerDependencies = JSON.parse(stdout)
Comment on lines +100 to +105
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


if (updateReact && nextVersionRequiresReact19(checkVersion)) {
const reactVersion = operation === 'update' ? REACT_19_VERSION : REACT_18_VERSION
const reactVersion =
operation === 'update' ? nextPeerDependencies['react'] : REACT_18_VERSION
packageJson.dependencies.react = reactVersion
packageJson.dependencies['react-dom'] = reactVersion
}
Expand Down
Loading