From 5c9052189d20766fa15d571f8848051513e3e07e Mon Sep 17 00:00:00 2001 From: Victor Berchet Date: Wed, 29 Jan 2025 11:55:36 +0100 Subject: [PATCH] refactor: Make the list of optional dependencies configurable (#297) --- .changeset/fresh-walls-hug.md | 5 + .../cloudflare/src/cli/build/bundle-server.ts | 24 ++-- .../build/patches/ast/optional-deps.spec.ts | 80 +++++++++-- .../cli/build/patches/ast/optional-deps.ts | 53 ++++--- .../patches/to-investigate/wrangler-deps.ts | 134 ------------------ 5 files changed, 120 insertions(+), 176 deletions(-) create mode 100644 .changeset/fresh-walls-hug.md diff --git a/.changeset/fresh-walls-hug.md b/.changeset/fresh-walls-hug.md new file mode 100644 index 00000000..79f4d694 --- /dev/null +++ b/.changeset/fresh-walls-hug.md @@ -0,0 +1,5 @@ +--- +"@opennextjs/cloudflare": patch +--- + +refactor: Make the list of optional dependencies configurable diff --git a/packages/cloudflare/src/cli/build/bundle-server.ts b/packages/cloudflare/src/cli/build/bundle-server.ts index db993820..4d7a5ff8 100644 --- a/packages/cloudflare/src/cli/build/bundle-server.ts +++ b/packages/cloudflare/src/cli/build/bundle-server.ts @@ -17,6 +17,20 @@ import { normalizePath, patchCodeWithValidations } from "./utils/index.js"; /** The dist directory of the Cloudflare adapter package */ const packageDistDir = path.join(path.dirname(fileURLToPath(import.meta.url)), "../.."); +/** + * List of optional Next.js dependencies. + * They are not required for Next.js to run but only needed to enabled specific features. + * When one of those dependency is required, it should be installed by the application. + */ +const optionalDependencies = [ + "caniuse-lite", + "critters", + "jimp", + "probe-image-size", + // `server.edge` is not available in react-dom@18 + "react-dom/server.edge", +]; + /** * Bundle the Open Next server. */ @@ -56,13 +70,7 @@ export async function bundleServer(buildOpts: BuildOptions): Promise { inlineRequirePagePlugin(buildOpts), setWranglerExternal(), ], - external: [ - "./middleware/handler.mjs", - // Next optional dependencies. - "caniuse-lite", - "jimp", - "probe-image-size", - ], + external: ["./middleware/handler.mjs", ...optionalDependencies], alias: { // Note: we apply an empty shim to next/dist/compiled/ws because it generates two `eval`s: // eval("require")("bufferutil"); @@ -196,7 +204,7 @@ async function updateWorkerBundledCode(workerOutputFile: string, buildOpts: Buil const bundle = parse(Lang.TypeScript, patchedCode).root(); - const { edits } = patchOptionalDependencies(bundle); + const { edits } = patchOptionalDependencies(bundle, optionalDependencies); await writeFile(workerOutputFile, bundle.commitEdits(edits)); } diff --git a/packages/cloudflare/src/cli/build/patches/ast/optional-deps.spec.ts b/packages/cloudflare/src/cli/build/patches/ast/optional-deps.spec.ts index 952543ed..57cd37ee 100644 --- a/packages/cloudflare/src/cli/build/patches/ast/optional-deps.spec.ts +++ b/packages/cloudflare/src/cli/build/patches/ast/optional-deps.spec.ts @@ -1,12 +1,12 @@ import { describe, expect, it } from "vitest"; -import { optionalDepRule } from "./optional-deps.js"; +import { buildOptionalDepRule } from "./optional-deps.js"; import { patchCode } from "./util.js"; describe("optional dependecy", () => { it('should wrap a top-level require("caniuse-lite") in a try-catch', () => { const code = `t = require("caniuse-lite");`; - expect(patchCode(code, optionalDepRule)).toMatchInlineSnapshot(` + expect(patchCode(code, buildOptionalDepRule(["caniuse-lite"]))).toMatchInlineSnapshot(` "try { t = require("caniuse-lite"); } catch { @@ -17,7 +17,7 @@ describe("optional dependecy", () => { it('should wrap a top-level require("caniuse-lite/data") in a try-catch', () => { const code = `t = require("caniuse-lite/data");`; - expect(patchCode(code, optionalDepRule)).toMatchInlineSnapshot( + expect(patchCode(code, buildOptionalDepRule(["caniuse-lite"]))).toMatchInlineSnapshot( ` "try { t = require("caniuse-lite/data"); @@ -30,7 +30,7 @@ describe("optional dependecy", () => { it('should wrap e.exports = require("caniuse-lite") in a try-catch', () => { const code = 'e.exports = require("caniuse-lite");'; - expect(patchCode(code, optionalDepRule)).toMatchInlineSnapshot(` + expect(patchCode(code, buildOptionalDepRule(["caniuse-lite"]))).toMatchInlineSnapshot(` "try { e.exports = require("caniuse-lite"); } catch { @@ -41,7 +41,7 @@ describe("optional dependecy", () => { it('should wrap module.exports = require("caniuse-lite") in a try-catch', () => { const code = 'module.exports = require("caniuse-lite");'; - expect(patchCode(code, optionalDepRule)).toMatchInlineSnapshot(` + expect(patchCode(code, buildOptionalDepRule(["caniuse-lite"]))).toMatchInlineSnapshot(` "try { module.exports = require("caniuse-lite"); } catch { @@ -52,7 +52,7 @@ describe("optional dependecy", () => { it('should wrap exports.foo = require("caniuse-lite") in a try-catch', () => { const code = 'exports.foo = require("caniuse-lite");'; - expect(patchCode(code, optionalDepRule)).toMatchInlineSnapshot(` + expect(patchCode(code, buildOptionalDepRule(["caniuse-lite"]))).toMatchInlineSnapshot(` "try { exports.foo = require("caniuse-lite"); } catch { @@ -63,23 +63,27 @@ describe("optional dependecy", () => { it('should not wrap require("lodash") in a try-catch', () => { const code = 't = require("lodash");'; - expect(patchCode(code, optionalDepRule)).toMatchInlineSnapshot(`"t = require("lodash");"`); + expect(patchCode(code, buildOptionalDepRule(["caniuse-lite"]))).toMatchInlineSnapshot( + `"t = require("lodash");"` + ); }); it('should not wrap require("other-module") if it does not match caniuse-lite regex', () => { const code = 't = require("other-module");'; - expect(patchCode(code, optionalDepRule)).toMatchInlineSnapshot(`"t = require("other-module");"`); + expect(patchCode(code, buildOptionalDepRule(["caniuse-lite"]))).toMatchInlineSnapshot( + `"t = require("other-module");"` + ); }); it("should not wrap a require() call already inside a try-catch", () => { const code = ` try { - const t = require("caniuse-lite"); + t = require("caniuse-lite"); } catch {} `; - expect(patchCode(code, optionalDepRule)).toMatchInlineSnapshot(` + expect(patchCode(code, buildOptionalDepRule(["caniuse-lite"]))).toMatchInlineSnapshot(` "try { - const t = require("caniuse-lite"); + t = require("caniuse-lite"); } catch {} " `); @@ -88,14 +92,62 @@ try { it("should handle require with subpath and not wrap if already in try-catch", () => { const code = ` try { - const t = require("caniuse-lite/path"); + t = require("caniuse-lite/path"); } catch {} `; - expect(patchCode(code, optionalDepRule)).toMatchInlineSnapshot(` + expect(patchCode(code, buildOptionalDepRule(["caniuse-lite"]))).toMatchInlineSnapshot(` "try { - const t = require("caniuse-lite/path"); + t = require("caniuse-lite/path"); } catch {} " `); }); + + it("should handle multiple dependencies", () => { + const code = ` +t1 = require("caniuse-lite"); +t2 = require("caniuse-lite/path"); +t3 = require("jimp"); +t4 = require("jimp/path"); +`; + expect(patchCode(code, buildOptionalDepRule(["caniuse-lite", "jimp"]))).toMatchInlineSnapshot(` + "try { + t1 = require("caniuse-lite"); + } catch { + throw new Error('The optional dependency "caniuse-lite" is not installed'); + }; + try { + t2 = require("caniuse-lite/path"); + } catch { + throw new Error('The optional dependency "caniuse-lite/path" is not installed'); + }; + try { + t3 = require("jimp"); + } catch { + throw new Error('The optional dependency "jimp" is not installed'); + }; + try { + t4 = require("jimp/path"); + } catch { + throw new Error('The optional dependency "jimp/path" is not installed'); + }; + " + `); + }); + + it("should not update partial matches", () => { + const code = ` +t1 = require("before-caniuse-lite"); +t2 = require("before-caniuse-lite/path"); +t3 = require("caniuse-lite-after"); +t4 = require("caniuse-lite-after/path"); +`; + expect(patchCode(code, buildOptionalDepRule(["caniuse-lite"]))).toMatchInlineSnapshot(` + "t1 = require("before-caniuse-lite"); + t2 = require("before-caniuse-lite/path"); + t3 = require("caniuse-lite-after"); + t4 = require("caniuse-lite-after/path"); + " + `); + }); }); diff --git a/packages/cloudflare/src/cli/build/patches/ast/optional-deps.ts b/packages/cloudflare/src/cli/build/patches/ast/optional-deps.ts index bfd9f200..b365d2a0 100644 --- a/packages/cloudflare/src/cli/build/patches/ast/optional-deps.ts +++ b/packages/cloudflare/src/cli/build/patches/ast/optional-deps.ts @@ -9,27 +9,40 @@ import { applyRule } from "./util.js"; * * So we wrap `require(optionalDep)` in a try/catch (if not already present). */ -export const optionalDepRule = ` -rule: - pattern: $$$LHS = require($$$REQ) - has: - pattern: $MOD - kind: string_fragment - stopBy: end - regex: ^(caniuse-lite|jimp|probe-image-size)(/|$) - not: - inside: - kind: try_statement +export function buildOptionalDepRule(dependencies: string[]) { + // Build a regexp matching either + // - the full packages names, i.e. `package` + // - subpaths in the package, i.e. `package/...` + const regex = `^(${dependencies.join("|")})(/|$)`; + return ` + rule: + pattern: $$$LHS = require($$$REQ) + has: + pattern: $MOD + kind: string_fragment stopBy: end + regex: ${regex} + not: + inside: + kind: try_statement + stopBy: end -fix: |- - try { - $$$LHS = require($$$REQ); - } catch { - throw new Error('The optional dependency "$MOD" is not installed'); - } -`; + fix: |- + try { + $$$LHS = require($$$REQ); + } catch { + throw new Error('The optional dependency "$MOD" is not installed'); + } + `; +} -export function patchOptionalDependencies(root: SgNode) { - return applyRule(optionalDepRule, root); +/** + * Wraps requires for passed dependencies in a `try ... catch`. + * + * @param root AST root node + * @param dependencies List of dependencies to wrap + * @returns matches and edits, see `applyRule` + */ +export function patchOptionalDependencies(root: SgNode, dependencies: string[]) { + return applyRule(buildOptionalDepRule(dependencies), root); } diff --git a/packages/cloudflare/src/cli/build/patches/to-investigate/wrangler-deps.ts b/packages/cloudflare/src/cli/build/patches/to-investigate/wrangler-deps.ts index 466895f2..cb5af596 100644 --- a/packages/cloudflare/src/cli/build/patches/to-investigate/wrangler-deps.ts +++ b/packages/cloudflare/src/cli/build/patches/to-investigate/wrangler-deps.ts @@ -2,9 +2,6 @@ import { readFileSync, writeFileSync } from "node:fs"; import { join } from "node:path"; import { type BuildOptions, getPackagePath } from "@opennextjs/aws/build/helper.js"; -import * as ts from "ts-morph"; - -import { tsParseFile } from "../../utils/index.js"; export function patchWranglerDeps(buildOpts: BuildOptions) { console.log("# patchWranglerDeps"); @@ -18,24 +15,6 @@ export function patchWranglerDeps(buildOpts: BuildOptions) { "node_modules/next/dist" ); - const pagesRuntimeFile = join(nextDistDir, "compiled/next-server/pages.runtime.prod.js"); - - const patchedPagesRuntime = readFileSync(pagesRuntimeFile, "utf-8").replace( - `e.exports=require("critters")`, - ` -try { - e.exports=require("critters"); -} -catch { - console.error('critters is not installed'); -} -` - ); - - writeFileSync(pagesRuntimeFile, patchedPagesRuntime); - - patchRequireReactDomServerEdge(nextDistDir); - // we shim @opentelemetry/api to the throwing shim so that it will throw right away, this is so that we throw inside the // try block here: https://github.com/vercel/next.js/blob/9e8266a7/packages/next/src/server/lib/trace/tracer.ts#L27-L31 // causing the code to require the 'next/dist/compiled/@opentelemetry/api' module instead (which properly works) @@ -48,116 +27,3 @@ catch { writeFileSync(tracerFile, patchedTracer); } - -/** - * `react-dom` v>=19 has a `server.edge` export: https://github.com/facebook/react/blob/a160102f3/packages/react-dom/package.json#L79 - * but version of `react-dom` <= 18 do not have this export but have a `server.browser` export instead: https://github.com/facebook/react/blob/8a015b68/packages/react-dom/package.json#L49 - * - * Next.js also try-catches importing the `server.edge` export: - * https://github.com/vercel/next.js/blob/6784575/packages/next/src/server/ReactDOMServerPages.js - * - * The issue here is that in the `.next/standalone/node_modules/next/dist/compiled/next-server/pages.runtime.prod.js` - * file for whatever reason there is a non `try-catch`ed require for the `server.edge` export - * - * This functions fixes this issue by wrapping the require in a try-catch block in the same way Next.js does it - * (note: this will make the build succeed but doesn't guarantee that everything will necessarily work at runtime since - * it's not clear what code and how might be rely on this require call) - * - */ -function patchRequireReactDomServerEdge(nextDistDir: string) { - // Patch .next/standalone/node_modules/next/dist/compiled/next-server/pages.runtime.prod.js - const pagesRuntimeFile = join(nextDistDir, "compiled/next-server/pages.runtime.prod.js"); - - const code = readFileSync(pagesRuntimeFile, "utf-8"); - const file = tsParseFile(code); - - // we need to update this function: `e=>{"use strict";e.exports=require("react-dom/server.edge")}` - file.getDescendantsOfKind(ts.SyntaxKind.ArrowFunction).forEach((arrowFunction) => { - // the function has a single parameter - const p = arrowFunction.getParameters(); - if (p.length !== 1) { - return; - } - const parameterName = p[0]!.getName(); - const bodyChildren = arrowFunction.getBody().getChildren(); - if ( - !( - bodyChildren.length === 3 && - bodyChildren[0]!.getFullText() === "{" && - bodyChildren[2]!.getFullText() === "}" - ) - ) { - return; - } - const bodyStatements = bodyChildren[1]?.getChildren(); - - // the function has only two statements: "use strict" and e.exports=require("react-dom/server.edge") - if ( - !( - bodyStatements?.length === 2 && - bodyStatements.every((statement) => statement.isKind(ts.SyntaxKind.ExpressionStatement)) - ) - ) { - return; - } - const bodyExpressionStatements = bodyStatements as [ts.ExpressionStatement, ts.ExpressionStatement]; - - const stringLiteralExpression = bodyExpressionStatements[0].getExpressionIfKind( - ts.SyntaxKind.StringLiteral - ); - - // the first statement needs to be "use strict" - if (stringLiteralExpression?.getText() !== '"use strict"') { - return; - } - - // the second statement (e.exports=require("react-dom/server.edge")) needs to be a binary expression - const binaryExpression = bodyExpressionStatements[1].getExpressionIfKind(ts.SyntaxKind.BinaryExpression); - if (!binaryExpression?.getOperatorToken().isKind(ts.SyntaxKind.EqualsToken)) { - return; - } - - // on the left we have `${parameterName}.exports` - const binaryLeft = binaryExpression.getLeft(); - if ( - !binaryLeft.isKind(ts.SyntaxKind.PropertyAccessExpression) || - binaryLeft.getExpressionIfKind(ts.SyntaxKind.Identifier)?.getText() !== parameterName || - binaryLeft.getName() !== "exports" - ) { - return; - } - - // on the right we have `require("react-dom/server.edge")` - const binaryRight = binaryExpression.getRight(); - if ( - !binaryRight.isKind(ts.SyntaxKind.CallExpression) || - binaryRight.getExpressionIfKind(ts.SyntaxKind.Identifier)?.getText() !== "require" - ) { - return; - } - const requireArgs = binaryRight.getArguments(); - if (requireArgs.length !== 1 || requireArgs[0]!.getText() !== '"react-dom/server.edge"') { - return; - } - - arrowFunction.setBodyText(` -// OpenNext patch -let ReactDOMServer; -try { - ReactDOMServer = require('react-dom/server.edge'); -} catch (error) { - if ( - error.code !== 'MODULE_NOT_FOUND' && - error.code !== 'ERR_PACKAGE_PATH_NOT_EXPORTED' - ) { - throw error; - } - ReactDOMServer = require('react-dom/server.browser'); -} -${parameterName}.exports = ReactDOMServer; -`); - }); - - const updatedCode = file.print(); - writeFileSync(pagesRuntimeFile, updatedCode); -}