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: create cache entries for fallback pages to support next@canary #2649

71 changes: 65 additions & 6 deletions src/build/content/prerendered.ts
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,
Expand Down Expand Up @@ -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
pieh marked this conversation as resolved.
Show resolved Hide resolved
*/
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,
})
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Expand All @@ -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)
}
Expand Down
1 change: 1 addition & 0 deletions src/run/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,4 @@ export const MODULE_DIR = fileURLToPath(new URL('.', import.meta.url))
export const PLUGIN_DIR = resolve(`${MODULE_DIR}../../..`)
// a file where we store the required-server-files config object in to access during runtime
export const RUN_CONFIG = 'run-config.json'
export const FS_BLOBS_MANIFEST = 'fs-blobs-manifest.json'
2 changes: 1 addition & 1 deletion src/run/handlers/request-context.cts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ export type RequestContext = {
responseCacheGetLastModified?: number
responseCacheKey?: string
responseCacheTags?: string[]
usedFsRead?: boolean
usedFsReadForNonFallback?: boolean
Copy link
Contributor

Choose a reason for hiding this comment

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

not blocking, but consider calling this something like usedFsReadForStaticRoute

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 think I'll merge this as-is to get this over with.

Minor side comment is that it hopefully now is actually functionally equivalent ( usedFsReadForNonFallback <=> usedFsReadForStaticRoute) but there's a chance there is another case here that we will be trying to handle in the future :D In this case I can at least say that usedFsReadForNonFallback name is actually correct because it literally check if html is fallback or not, but I am actually not 100% sure that all non-fallbacks are actually "StaticRoutes" heh

didPagesRouterOnDemandRevalidate?: boolean
serverTiming?: string
routeHandlerRevalidate?: NetlifyCachedRouteValue['revalidate']
Expand Down
8 changes: 4 additions & 4 deletions src/run/headers.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -321,7 +321,7 @@ describe('headers', () => {
})
})

test('should not set any headers if "cache-control" is not set and "requestContext.usedFsRead" is not truthy', () => {
test('should not set any headers if "cache-control" is not set and "requestContext.usedFsReadForNonFallback" is not truthy', () => {
const headers = new Headers()
const request = new Request(defaultUrl)
vi.spyOn(headers, 'set')
Expand All @@ -331,15 +331,15 @@ describe('headers', () => {
expect(headers.set).toHaveBeenCalledTimes(0)
})

test('should set permanent, durable "netlify-cdn-cache-control" if "cache-control" is not set and "requestContext.usedFsRead" is truthy', () => {
test('should set permanent, durable "netlify-cdn-cache-control" if "cache-control" is not set and "requestContext.usedFsReadForNonFallback" is truthy', () => {
const headers = new Headers()
const request = new Request(defaultUrl)
vi.spyOn(headers, 'set')

const requestContext = createRequestContext()
requestContext.usedFsRead = true
requestContext.usedFsReadForNonFallback = true

setCacheControlHeaders(headers, request, requestContext, true)
setCacheControlHeaders(headers, request, requestContext)

expect(headers.set).toHaveBeenNthCalledWith(
1,
Expand Down
7 changes: 5 additions & 2 deletions src/run/headers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand All @@ -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'))
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 to prevent setting tags on uncacheable responses when fallback in next@canary is served

) {
headers.set('netlify-cache-tag', requestContext.responseCacheTags.join(','))
}
}
Expand Down
37 changes: 31 additions & 6 deletions src/run/next.cts
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'
Expand Down Expand Up @@ -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 () => {
Expand All @@ -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))
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 think this was not actually working correctly if user had non-default distDir (aka not .next) and also likely not working in monorepos

Copy link
Contributor

Choose a reason for hiding this comment

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

this makes sense, however the outputRoot seems to have nothing to do with blob manifest so I'm wondering if there should be a different way to specify this value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

given #2649 (comment) - I'll drop this change and keep hardcoded .next here as out of scope for this PR - I still think this is an issue, but I will not try to fix it in same PR that introduces creation of blob cache entries for fallbacks

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
Expand Down
Loading
Loading