Skip to content

fix: new ISR cache handling to resolve regression in 13.4 #2165

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 12 commits into from
Jun 16, 2023
Merged
Show file tree
Hide file tree
Changes from all 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
4 changes: 0 additions & 4 deletions packages/runtime/src/helpers/dev.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,11 @@ import type { OnPreBuild } from '@netlify/build'
import execa from 'execa'

import { writeDevEdgeFunction } from './edge'
import { patchNextFiles } from './files'

// The types haven't been updated yet
export const onPreDev: OnPreBuild = async ({ constants, netlifyConfig }) => {
const base = netlifyConfig.build.base ?? process.cwd()

// Need to patch the files, because build might not have been run
await patchNextFiles(base)

await writeDevEdgeFunction(constants)
// Don't await this or it will never finish
execa.node(
Expand Down
128 changes: 1 addition & 127 deletions packages/runtime/src/helpers/files.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,7 @@ import { cpus } from 'os'

import type { NetlifyConfig } from '@netlify/build'
import { yellowBright } from 'chalk'
import {
existsSync,
readJson,
move,
copy,
writeJson,
readFile,
writeFile,
ensureDir,
readFileSync,
remove,
} from 'fs-extra'
import { existsSync, readJson, move, copy, writeJson, ensureDir, readFileSync, remove } from 'fs-extra'
import globby from 'globby'
import { PrerenderManifest } from 'next/dist/build'
import { outdent } from 'outdent'
Expand Down Expand Up @@ -297,45 +286,6 @@ export const moveStaticPages = async ({
}
}

const PATCH_WARNING = `/* File patched by Netlify */`

/**
* Attempt to patch a source file, preserving a backup
*/
const patchFile = async ({
file,
replacements,
}: {
file: string
replacements: Array<[from: string, to: string]>
}): Promise<boolean> => {
if (!existsSync(file)) {
console.warn('File was not found')
return false
}
let content = await readFile(file, 'utf8')

// If the file has already been patched, patch the backed-up original instead
if (content.includes(PATCH_WARNING) && existsSync(`${file}.orig`)) {
content = await readFile(`${file}.orig`, 'utf8')
}

const newContent = replacements.reduce((acc, [from, to]) => {
if (acc.includes(to)) {
console.log('Already patched. Skipping.')
return acc
}
return acc.replace(from, to)
}, content)
if (newContent === content) {
console.warn('File was not changed')
return false
}
await writeFile(`${file}.orig`, content)
await writeFile(file, `${newContent}\n${PATCH_WARNING}`)
console.log('Done')
return true
}
/**
* The file we need has moved around a bit over the past few versions,
* so we iterate through the options until we find it
Expand Down Expand Up @@ -386,82 +336,6 @@ export const getDependenciesOfFile = async (file: string) => {
return dependencies.files.map((dep) => resolve(dirname(file), dep))
}

const baseServerReplacements: Array<[string, string]> = [

Choose a reason for hiding this comment

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

We no longer need the server patches her because we handle this in NetlifyNextServer now.

// force manual revalidate during cache fetches
// for more info https://github.com/netlify/next-runtime/pull/1541
[
`checkIsManualRevalidate(req, this.renderOpts.previewProps)`,
`checkIsManualRevalidate(process.env._REVALIDATE_SSG ? { headers: { 'x-prerender-revalidate': this.renderOpts.previewProps.previewModeId } } : req, this.renderOpts.previewProps)`,
],
// In https://github.com/vercel/next.js/pull/47803 checkIsManualRevalidate was renamed to checkIsOnDemandRevalidate
[
`checkIsOnDemandRevalidate(req, this.renderOpts.previewProps)`,
`checkIsOnDemandRevalidate(process.env._REVALIDATE_SSG ? { headers: { 'x-prerender-revalidate': this.renderOpts.previewProps.previewModeId } } : req, this.renderOpts.previewProps)`,
],
// format of checkIsOnDemandRevalidate changed in 13.3.4
[
'checkIsOnDemandRevalidate)(req, this.renderOpts.previewProps)',
'checkIsOnDemandRevalidate)(process.env._REVALIDATE_SSG ? { headers: { "x-prerender-revalidate": this.renderOpts.previewProps.previewModeId } } : req, this.renderOpts.previewProps)',
],
// ensure ISR 404 pages send the correct SWR cache headers
[`private: isPreviewMode || is404Page && cachedData`, `private: isPreviewMode && cachedData`],
]

const nextServerReplacements: Array<[string, string]> = [
[
`getMiddlewareManifest() {\n if (this.minimalMode) return null;`,
`getMiddlewareManifest() {\n if (this.minimalMode || (!process.env.NETLIFY_DEV && process.env.NEXT_DISABLE_NETLIFY_EDGE !== 'true' && process.env.NEXT_DISABLE_NETLIFY_EDGE !== '1')) return null;`,
],
[
`generateCatchAllMiddlewareRoute(devReady) {\n if (this.minimalMode) return []`,
`generateCatchAllMiddlewareRoute(devReady) {\n if (this.minimalMode || (!process.env.NETLIFY_DEV && process.env.NEXT_DISABLE_NETLIFY_EDGE !== 'true' && process.env.NEXT_DISABLE_NETLIFY_EDGE !== '1')) return [];`,
],
[
`generateCatchAllMiddlewareRoute() {\n if (this.minimalMode) return undefined;`,
`generateCatchAllMiddlewareRoute() {\n if (this.minimalMode || (!process.env.NETLIFY_DEV && process.env.NEXT_DISABLE_NETLIFY_EDGE !== 'true' && process.env.NEXT_DISABLE_NETLIFY_EDGE !== '1')) return undefined;`,
],
[
`getMiddlewareManifest() {\n if (this.minimalMode) {`,
`getMiddlewareManifest() {\n if (!this.minimalMode && (!process.env.NETLIFY_DEV && process.env.NEXT_DISABLE_NETLIFY_EDGE === 'true' || process.env.NEXT_DISABLE_NETLIFY_EDGE === '1')) {`,
],
]

export const patchNextFiles = async (root: string): Promise<void> => {
const baseServerFile = getServerFile(root)
console.log(`Patching ${baseServerFile}`)
if (baseServerFile) {
await patchFile({
file: baseServerFile,
replacements: baseServerReplacements,
})
}

const nextServerFile = getServerFile(root, false)
console.log(`Patching ${nextServerFile}`)
if (nextServerFile) {
await patchFile({
file: nextServerFile,
replacements: nextServerReplacements,
})
}
}

export const unpatchFile = async (file: string): Promise<void> => {
const origFile = `${file}.orig`
if (existsSync(origFile)) {
await move(origFile, file, { overwrite: true })
}
}

export const unpatchNextFiles = async (root: string): Promise<void> => {
const baseServerFile = getServerFile(root)
await unpatchFile(baseServerFile)
const nextServerFile = getServerFile(root, false)
if (nextServerFile !== baseServerFile) {
await unpatchFile(nextServerFile)
}
}

export const movePublicFiles = async ({
appDir,
outdir,
Expand Down
4 changes: 1 addition & 3 deletions packages/runtime/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import {
} from './helpers/config'
import { onPreDev } from './helpers/dev'
import { writeEdgeFunctions, loadMiddlewareManifest, cleanupEdgeFunctions } from './helpers/edge'
import { moveStaticPages, movePublicFiles, patchNextFiles, removeMetadataFiles } from './helpers/files'
import { moveStaticPages, movePublicFiles, removeMetadataFiles } from './helpers/files'
import { splitApiRoutes } from './helpers/flags'
import {
generateFunctions,
Expand Down Expand Up @@ -184,8 +184,6 @@ const plugin: NetlifyPlugin = {

await movePublicFiles({ appDir, outdir, publish, basePath })

await patchNextFiles(appDir)

if (!destr(process.env.SERVE_STATIC_FILES_FROM_ORIGIN)) {
await moveStaticPages({ target, netlifyConfig, i18n, basePath })
}
Expand Down
3 changes: 0 additions & 3 deletions packages/runtime/src/templates/getHandler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,9 +69,6 @@ const makeHandler = ({ conf, app, pageRoot, NextServer, staticManifest = [], mod

// We don't want to write ISR files to disk in the lambda environment
conf.experimental.isrFlushToDisk = false
// This is our flag that we use when patching the source
// eslint-disable-next-line no-underscore-dangle
process.env._REVALIDATE_SSG = 'true'

Choose a reason for hiding this comment

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

This was used for the server patches so is no longer required.

for (const [key, value] of Object.entries(conf.env)) {
process.env[key] = String(value)
}
Expand Down
18 changes: 15 additions & 3 deletions packages/runtime/src/templates/server.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { PrerenderManifest } from 'next/dist/build'
import type { BaseNextResponse } from 'next/dist/server/base-http'
import { NodeRequestHandler, Options } from 'next/dist/server/next-server'
import type { NodeRequestHandler, Options } from 'next/dist/server/next-server'

import {
netlifyApiFetch,
Expand Down Expand Up @@ -42,6 +42,7 @@ const getNetlifyNextServer = (NextServer: NextServerType) => {
// conditionally use the prebundled React module
this.netlifyPrebundleReact(url)

// intercept on-demand revalidation requests and handle with the Netlify API
if (headers['x-prerender-revalidate'] && this.netlifyConfig.revalidateToken) {
// handle on-demand revalidation by purging the ODB cache
await this.netlifyRevalidate(url)
Expand All @@ -50,9 +51,20 @@ const getNetlifyNextServer = (NextServer: NextServerType) => {
res.statusCode = 200
res.setHeader('x-nextjs-cache', 'REVALIDATED')
res.send()
} else {
return handler(req, res, parsedUrl)
return
}

// force Next to revalidate all requests so that we always have fresh content
// for our ODBs and middleware is disabled at the origin
// but ignore in preview mode (prerender_bypass is set to true in preview mode)
// because otherwise revalidate will override preview mode
if (!headers.cookie?.includes('__prerender_bypass')) {
// this header controls whether Next.js will revalidate the page
// and needs to be set to the preview mode id to enable it
headers['x-prerender-revalidate'] = this.renderOpts.previewProps.previewModeId
}

return handler(req, res, parsedUrl)
}
}

Expand Down
24 changes: 0 additions & 24 deletions test/helpers/files.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,6 @@ import {
stripLocale,
matchesRedirect,
matchesRewrite,
patchNextFiles,
unpatchNextFiles,
getDependenciesOfFile,
getSourceFileForPage,
} from '../../packages/runtime/src/helpers/files'
Expand Down Expand Up @@ -189,28 +187,6 @@ describe('files utility functions', () => {
})
})

describeCwdTmpDir('file patching', () => {

Choose a reason for hiding this comment

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

Tests are obsolete as we no longer patch server files.

it('patches Next server files', async () => {
// Testing to make sure that the patching functionality works within base-server.js and next-server.js files
const root = path.resolve(dirname(resolve(__dirname, '..')))
await copy(join(root, 'package.json'), path.join(process.cwd(), 'package.json'))
await ensureDir(path.join(process.cwd(), 'node_modules'))
await copy(path.join(root, 'node_modules', 'next'), path.join(process.cwd(), 'node_modules', 'next'))

await patchNextFiles(process.cwd())
const serverFile = path.resolve(process.cwd(), 'node_modules', 'next', 'dist', 'server', 'base-server.js')
const patchedData = await readFileSync(serverFile, 'utf8')
expect(patchedData.includes('_REVALIDATE_SSG')).toBeTruthy()
expect(patchedData.includes('private: isPreviewMode && cachedData')).toBeTruthy()

await unpatchNextFiles(process.cwd())

const unPatchedData = await readFileSync(serverFile, 'utf8')
expect(unPatchedData.includes('_REVALIDATE_SSG')).toBeFalsy()
expect(unPatchedData.includes('private: isPreviewMode && cachedData')).toBeFalsy()
})
})

describe('dependency tracing', () => {
it('generates dependency list from a source file', async () => {
const dependencies = await getDependenciesOfFile(resolve(__dirname, '../fixtures/analysis/background.js'))
Expand Down