-
Notifications
You must be signed in to change notification settings - Fork 89
fix: create cache entries for fallback pages to support next@canary #2649
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
Changes from 3 commits
4b4b9a7
057d339
df703e8
f9b4de6
724bd32
e5de50e
43b8488
e8951a8
9169bd5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,13 +1,16 @@ | ||
import { existsSync } from 'node:fs' | ||
import { mkdir, readFile, writeFile } from 'node:fs/promises' | ||
import { join } from 'node:path' | ||
import { join as posixJoin } from 'node:path/posix' | ||
|
||
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 { FS_BLOBS_MANIFEST } from '../../run/constants.js' | ||
import { type FSBlobsManifest } from '../../run/next.cjs' | ||
import { encodeBlobKey } from '../../shared/blobkey.js' | ||
import type { | ||
CachedFetchValueForMultipleVersions, | ||
|
@@ -41,17 +44,28 @@ const writeCacheEntry = async ( | |
} | ||
|
||
/** | ||
* Normalize routes by stripping leading slashes and ensuring root path is index | ||
* Normalize routes by ensuring leading slashes and ensuring root path is index | ||
*/ | ||
const routeToFilePath = (path: string) => (path === '/' ? '/index' : path) | ||
const routeToFilePath = (path: string) => { | ||
if (path === '/') { | ||
return '/index' | ||
} | ||
|
||
if (path.startsWith('/')) { | ||
return path | ||
} | ||
|
||
return `/${path}` | ||
} | ||
|
||
const buildPagesCacheValue = async ( | ||
path: string, | ||
shouldUseEnumKind: boolean, | ||
shouldSkipJson = false, | ||
): Promise<NetlifyCachedPageValue> => ({ | ||
kind: shouldUseEnumKind ? 'PAGES' : 'PAGE', | ||
html: await readFile(`${path}.html`, 'utf-8'), | ||
pageData: JSON.parse(await readFile(`${path}.json`, 'utf-8')), | ||
pageData: shouldSkipJson ? {} : JSON.parse(await readFile(`${path}.json`, 'utf-8')), | ||
headers: undefined, | ||
status: undefined, | ||
}) | ||
|
@@ -146,8 +160,13 @@ export const copyPrerenderedContent = async (ctx: PluginContext): Promise<void> | |
}) | ||
: false | ||
|
||
await Promise.all( | ||
Object.entries(manifest.routes).map( | ||
const fsBlobsManifest: FSBlobsManifest = { | ||
fallbackPaths: [], | ||
outputRoot: ctx.distDir, | ||
} | ||
|
||
await Promise.all([ | ||
...Object.entries(manifest.routes).map( | ||
([route, meta]): Promise<void> => | ||
limitConcurrentPrerenderContentHandling(async () => { | ||
const lastModified = meta.initialRevalidateSeconds | ||
|
@@ -195,7 +214,43 @@ export const copyPrerenderedContent = async (ctx: PluginContext): Promise<void> | |
await writeCacheEntry(key, value, lastModified, ctx) | ||
}), | ||
), | ||
) | ||
...Object.entries(manifest.dynamicRoutes).map(async ([route, meta]) => { | ||
// fallback can be `string | false | null` | ||
// - `string` - when user use pages router with `fallback: true`, and then it's html file path | ||
// - `null` - when user use pages router with `fallback: 'block'` or app router with `export const dynamicParams = true` | ||
// - `false` - when user use pages router with `fallback: false` or app router with `export const dynamicParams = false` | ||
if (typeof meta.fallback === 'string') { | ||
// https://github.com/vercel/next.js/pull/68603 started using route cache to serve fallbacks | ||
// so we have to seed blobs with fallback entries | ||
|
||
// create cache entry for pages router with `fallback: true` case | ||
await limitConcurrentPrerenderContentHandling(async () => { | ||
// dynamic routes don't have entries for each locale so we have to generate them | ||
// ourselves. If i18n is not used we use empty string as "locale" to be able to use | ||
// same handling wether i18n is used or not | ||
const locales = ctx.buildConfig.i18n?.locales ?? [''] | ||
|
||
const lastModified = Date.now() | ||
for (const locale of locales) { | ||
const key = routeToFilePath(posixJoin(locale, route)) | ||
const value = await buildPagesCacheValue( | ||
join(ctx.publishDir, 'server/pages', key), | ||
shouldUseEnumKind, | ||
true, // there is no corresponding json file for fallback, so we are skipping it for this entry | ||
) | ||
// Netlify Forms are not support and require a workaround | ||
if (value.kind === 'PAGE' || value.kind === 'PAGES' || value.kind === 'APP_PAGE') { | ||
verifyNetlifyForms(ctx, value.html) | ||
} | ||
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. not a blocker, but i'd argue that the Netlify Forms verification here is not adding much in terms of user value and we can strip it out to reduce noise (also APP_PAGE can never be true) 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. Yeah, I'll drop that completely form here - we actually already are doing this right now because of https://github.com/netlify/next-runtime/blob/5f5ec0651726abf1027319707beaa1580a0c5dd8/src/build/content/static.ts#L35 so this was just doing same check again on same content This was just copied from above code heh |
||
|
||
await writeCacheEntry(key, value, lastModified, ctx) | ||
|
||
fsBlobsManifest.fallbackPaths.push(`${key}.html`) | ||
} | ||
}) | ||
} | ||
}), | ||
]) | ||
|
||
// app router 404 pages are not in the prerender manifest | ||
// so we need to check for them manually | ||
|
@@ -208,6 +263,10 @@ export const copyPrerenderedContent = async (ctx: PluginContext): Promise<void> | |
) | ||
await writeCacheEntry(key, value, lastModified, ctx) | ||
} | ||
await writeFile( | ||
join(ctx.serverHandlerDir, FS_BLOBS_MANIFEST), | ||
JSON.stringify(fsBlobsManifest), | ||
) | ||
} catch (error) { | ||
ctx.failBuild('Failed assembling prerendered content for upload', error) | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,7 +11,7 @@ export type RequestContext = { | |
responseCacheGetLastModified?: number | ||
responseCacheKey?: string | ||
responseCacheTags?: string[] | ||
usedFsRead?: boolean | ||
usedFsReadForNonFallback?: boolean | ||
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. not blocking, but consider calling this 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 think I'll merge this as-is to get this over with. Minor side comment is that it hopefully now is actually functionally equivalent ( |
||
didPagesRouterOnDemandRevalidate?: boolean | ||
serverTiming?: string | ||
routeHandlerRevalidate?: NetlifyCachedRouteValue['revalidate'] | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -263,7 +263,7 @@ export const setCacheControlHeaders = ( | |
cacheControl === null && | ||
!headers.has('cdn-cache-control') && | ||
!headers.has('netlify-cdn-cache-control') && | ||
requestContext.usedFsRead | ||
requestContext.usedFsReadForNonFallback | ||
) { | ||
// handle CDN Cache Control on static files | ||
headers.set('cache-control', 'public, max-age=0, must-revalidate') | ||
|
@@ -272,7 +272,10 @@ export const setCacheControlHeaders = ( | |
} | ||
|
||
export const setCacheTagsHeaders = (headers: Headers, requestContext: RequestContext) => { | ||
if (requestContext.responseCacheTags) { | ||
if ( | ||
requestContext.responseCacheTags && | ||
(headers.has('cache-control') || headers.has('netlify-cdn-cache-control')) | ||
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 to prevent setting tags on uncacheable responses when fallback in next@canary is served |
||
) { | ||
headers.set('netlify-cache-tag', requestContext.responseCacheTags.join(',')) | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
import fs from 'fs/promises' | ||
import { relative, resolve } from 'path' | ||
import fs, { readFile } from 'fs/promises' | ||
import { join, relative, resolve } from 'path' | ||
|
||
// @ts-expect-error no types installed | ||
import { patchFs } from 'fs-monkey' | ||
|
@@ -80,6 +80,27 @@ console.timeEnd('import next server') | |
|
||
type FS = typeof import('fs') | ||
|
||
export type FSBlobsManifest = { | ||
fallbackPaths: string[] | ||
outputRoot: string | ||
} | ||
|
||
function normalizeStaticAssetPath(path: string) { | ||
return path.startsWith('/') ? path : `/${path}` | ||
} | ||
|
||
let fsBlobsManifestPromise: Promise<FSBlobsManifest> | undefined | ||
const getFSBlobsManifest = (): Promise<FSBlobsManifest> => { | ||
if (!fsBlobsManifestPromise) { | ||
fsBlobsManifestPromise = (async () => { | ||
const { FS_BLOBS_MANIFEST, PLUGIN_DIR } = await import('./constants.js') | ||
return JSON.parse(await readFile(resolve(PLUGIN_DIR, FS_BLOBS_MANIFEST), 'utf-8')) | ||
})() | ||
} | ||
|
||
return fsBlobsManifestPromise | ||
} | ||
|
||
export async function getMockedRequestHandlers(...args: Parameters<typeof getRequestHandlers>) { | ||
const tracer = getTracer() | ||
return tracer.withActiveSpan('mocked request handler', async () => { | ||
|
@@ -96,13 +117,17 @@ export async function getMockedRequestHandlers(...args: Parameters<typeof getReq | |
} catch (error) { | ||
// only try to get .html files from the blob store | ||
if (typeof path === 'string' && path.endsWith('.html')) { | ||
const fsBlobsManifest = await getFSBlobsManifest() | ||
|
||
const store = getRegionalBlobStore() | ||
const relPath = relative(resolve('.next/server/pages'), path) | ||
const relPath = relative(resolve(join(fsBlobsManifest.outputRoot, '/server/pages')), path) | ||
const file = await store.get(await encodeBlobKey(relPath)) | ||
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 think this was not actually working correctly if user had non-default 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 makes sense, however the 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. given #2649 (comment) - I'll drop this change and keep hardcoded |
||
if (file !== null) { | ||
const requestContext = getRequestContext() | ||
if (requestContext) { | ||
requestContext.usedFsRead = true | ||
if (!fsBlobsManifest.fallbackPaths.includes(normalizeStaticAssetPath(relPath))) { | ||
const requestContext = getRequestContext() | ||
if (requestContext) { | ||
requestContext.usedFsReadForNonFallback = true | ||
} | ||
} | ||
|
||
return file | ||
|
Uh oh!
There was an error while loading. Please reload this page.