-
Notifications
You must be signed in to change notification settings - Fork 89
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
Changes from 7 commits
e4a11ec
c867b59
70b73d8
7b7a2de
ce82ee7
478062b
14c8a35
0888742
e56bbd6
9bd37d0
6105a4a
6565c8a
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 |
---|---|---|
|
@@ -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' | ||
|
@@ -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 | ||
|
@@ -386,82 +336,6 @@ export const getDependenciesOfFile = async (file: string) => { | |
return dependencies.files.map((dep) => resolve(dirname(file), dep)) | ||
} | ||
|
||
const baseServerReplacements: Array<[string, string]> = [ | ||
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. 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, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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' | ||
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 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) | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,8 +8,6 @@ import { | |
stripLocale, | ||
matchesRedirect, | ||
matchesRewrite, | ||
patchNextFiles, | ||
unpatchNextFiles, | ||
getDependenciesOfFile, | ||
getSourceFileForPage, | ||
} from '../../packages/runtime/src/helpers/files' | ||
|
@@ -189,28 +187,6 @@ describe('files utility functions', () => { | |
}) | ||
}) | ||
|
||
describeCwdTmpDir('file patching', () => { | ||
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. 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')) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works now @orinokai. It was failing previously not finding
Bitten
which was expected pre the changes in this PR.