Skip to content

Commit a19b34d

Browse files
vicbvicb
authored
perf: reduce the amount of code converted to AST (#354)
We only patch files (vs the whole bundle) to patch instrumentation
1 parent a630aea commit a19b34d

15 files changed

+395
-222
lines changed

‎.changeset/eleven-pumas-film.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@opennextjs/cloudflare": patch
3+
---
4+
5+
perf: reduce CPU and memory usage by limiting code to AST parsing

‎packages/cloudflare/package.json‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,6 @@
7676
"@opennextjs/aws": "https://pkg.pr.new/@opennextjs/aws@727",
7777
"enquirer": "^2.4.1",
7878
"glob": "catalog:",
79-
"ts-morph": "catalog:",
8079
"yaml": "^2.7.0"
8180
},
8281
"peerDependencies": {

‎packages/cloudflare/src/cli/build/bundle-server.ts‎

Lines changed: 12 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -4,14 +4,16 @@ import path from "node:path";
44
import { fileURLToPath } from "node:url";
55

66
import { type BuildOptions, getPackagePath } from "@opennextjs/aws/build/helper.js";
7-
import { getCrossPlatformPathRegex } from "@opennextjs/aws/utils/regex.js";
8-
import { build, Plugin } from "esbuild";
7+
import { build } from "esbuild";
98

109
import { patchVercelOgLibrary } from "./patches/ast/patch-vercel-og-library.js";
1110
import { patchWebpackRuntime } from "./patches/ast/webpack-runtime.js";
1211
import * as patches from "./patches/index.js";
12+
import { ContentUpdater } from "./patches/plugins/content-updater.js";
13+
import { patchLoadInstrumentation } from "./patches/plugins/load-instrumentation.js";
1314
import { handleOptionalDependencies } from "./patches/plugins/optional-deps.js";
1415
import { fixRequire } from "./patches/plugins/require.js";
16+
import { shimRequireHook } from "./patches/plugins/require-hook.js";
1517
import { inlineRequirePagePlugin } from "./patches/plugins/require-page.js";
1618
import { setWranglerExternal } from "./patches/plugins/wrangler-external.js";
1719
import { normalizePath, patchCodeWithValidations } from "./utils/index.js";
@@ -58,6 +60,8 @@ export async function bundleServer(buildOpts: BuildOptions): Promise<void> {
5860
const openNextServer = path.join(outputPath, packagePath, `index.mjs`);
5961
const openNextServerBundle = path.join(outputPath, packagePath, `handler.mjs`);
6062

63+
const updater = new ContentUpdater();
64+
6165
const result = await build({
6266
entryPoints: [openNextServer],
6367
bundle: true,
@@ -77,11 +81,14 @@ export async function bundleServer(buildOpts: BuildOptions): Promise<void> {
7781
// - ESBuild `node` platform: https://esbuild.github.io/api/#platform
7882
conditions: [],
7983
plugins: [
80-
createFixRequiresESBuildPlugin(buildOpts),
81-
inlineRequirePagePlugin(buildOpts),
84+
shimRequireHook(buildOpts),
85+
inlineRequirePagePlugin(updater, buildOpts),
8286
setWranglerExternal(),
83-
fixRequire(),
87+
fixRequire(updater),
8488
handleOptionalDependencies(optionalDependencies),
89+
patchLoadInstrumentation(updater),
90+
// Apply updater updaters, must be the last plugin
91+
updater.plugin,
8592
],
8693
external: ["./middleware/handler.mjs"],
8794
alias: {
@@ -192,7 +199,6 @@ export async function updateWorkerBundledCode(
192199
(code) => patches.inlineMiddlewareManifestRequire(code, buildOpts),
193200
],
194201
["exception bubbling", patches.patchExceptionBubbling],
195-
["`loadInstrumentationModule` function", patches.patchLoadInstrumentationModule],
196202
[
197203
"`patchAsyncStorage` call",
198204
(code) =>
@@ -210,21 +216,6 @@ export async function updateWorkerBundledCode(
210216
await writeFile(workerOutputFile, patchedCode);
211217
}
212218

213-
function createFixRequiresESBuildPlugin(options: BuildOptions): Plugin {
214-
return {
215-
name: "replaceRelative",
216-
setup(build) {
217-
// Note: we (empty) shim require-hook modules as they generate problematic code that uses requires
218-
build.onResolve(
219-
{ filter: getCrossPlatformPathRegex(String.raw`^\./require-hook$`, { escape: false }) },
220-
() => ({
221-
path: path.join(options.outputDir, "cloudflare-templates/shims/empty.js"),
222-
})
223-
);
224-
},
225-
};
226-
}
227-
228219
/**
229220
* Gets the path of the worker.js file generated by the build process
230221
*

‎packages/cloudflare/src/cli/build/patches/plugins/content-updater.ts‎

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,65 @@
1+
/**
2+
* ESBuild stops calling `onLoad` hooks after the first hook returns an updated content.
3+
*
4+
* The updater allows multiple plugins to update the content.
5+
*/
6+
7+
import { readFile } from "node:fs/promises";
8+
9+
import { type OnLoadArgs, type OnLoadOptions, type Plugin, type PluginBuild } from "esbuild";
10+
11+
export type Callback = (args: {
12+
contents: string;
13+
path: string;
14+
}) => string | undefined | Promise<string | undefined>;
15+
export type Updater = OnLoadOptions & { callback: Callback };
16+
17+
export class ContentUpdater {
18+
updaters = new Map<string, Updater>();
19+
20+
/**
21+
* Register a callback to update the file content.
22+
*
23+
* The callbacks are called in order of registration.
24+
*
25+
* @param name The name of the plugin (must be unique).
26+
* @param options Same options as the `onLoad` hook to restrict updates.
27+
* @param callback The callback updating the content.
28+
* @returns A noop ESBuild plugin.
29+
*/
30+
updateContent(name: string, options: OnLoadOptions, callback: Callback): Plugin {
31+
if (this.updaters.has(name)) {
32+
throw new Error(`Plugin "${name}" already registered`);
33+
}
34+
this.updaters.set(name, { ...options, callback });
35+
return {
36+
name,
37+
setup() {},
38+
};
39+
}
40+
41+
/**
42+
* Returns an ESBuild plugin applying the registered updates.
43+
*/
44+
get plugin() {
45+
return {
46+
name: "aggregate-on-load",
47+
48+
setup: async (build: PluginBuild) => {
49+
build.onLoad({ filter: /\.(js|mjs|cjs|jsx|ts|tsx)$/ }, async (args: OnLoadArgs) => {
50+
let contents = await readFile(args.path, "utf-8");
51+
for (const { filter, namespace, callback } of this.updaters.values()) {
52+
if (namespace !== undefined && args.namespace !== namespace) {
53+
continue;
54+
}
55+
if (!filter.test(args.path)) {
56+
continue;
57+
}
58+
contents = (await callback({ contents, path: args.path })) ?? contents;
59+
}
60+
return { contents };
61+
});
62+
},
63+
};
64+
}
65+
}

‎packages/cloudflare/src/cli/build/patches/plugins/load-instrumentation.spec.ts‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
import { describe, expect, test } from "vitest";
2+
3+
import { patchCode } from "../ast/util.js";
4+
import { instrumentationRule } from "./load-instrumentation.js";
5+
6+
describe("LoadInstrumentationModule", () => {
7+
test("patch", () => {
8+
const code = `
9+
export default class NextNodeServer extends BaseServer<
10+
Options,
11+
NodeNextRequest,
12+
NodeNextResponse
13+
> {
14+
protected async loadInstrumentationModule() {
15+
if (!this.serverOptions.dev) {
16+
try {
17+
this.instrumentation = await dynamicRequire(
18+
resolve(
19+
this.serverOptions.dir || '.',
20+
this.serverOptions.conf.distDir!,
21+
'server',
22+
INSTRUMENTATION_HOOK_FILENAME
23+
)
24+
)
25+
} catch (err: any) {
26+
if (err.code !== 'MODULE_NOT_FOUND') {
27+
throw new Error(
28+
'An error occurred while loading the instrumentation hook',
29+
{ cause: err }
30+
)
31+
}
32+
}
33+
}
34+
return this.instrumentation
35+
}
36+
}`;
37+
38+
expect(patchCode(code, instrumentationRule)).toMatchInlineSnapshot(`
39+
"export default class NextNodeServer extends BaseServer<
40+
Options,
41+
NodeNextRequest,
42+
NodeNextResponse
43+
> {
44+
async loadInstrumentationModule() { }
45+
}"
46+
`);
47+
});
48+
});

‎packages/cloudflare/src/cli/build/patches/plugins/load-instrumentation.ts‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
/**
2+
* `loadInstrumentationModule` uses a dynamic require which is not supported.
3+
*/
4+
5+
import { patchCode } from "../ast/util.js";
6+
import type { ContentUpdater } from "./content-updater.js";
7+
8+
export const instrumentationRule = `
9+
rule:
10+
kind: method_definition
11+
all:
12+
- has: {field: name, regex: ^loadInstrumentationModule$}
13+
- has: {pattern: dynamicRequire, stopBy: end}
14+
15+
fix: async loadInstrumentationModule() { }
16+
`;
17+
18+
export function patchLoadInstrumentation(updater: ContentUpdater) {
19+
return updater.updateContent(
20+
"patch-load-instrumentation",
21+
{ filter: /\.(js|mjs|cjs|jsx|ts|tsx)$/ },
22+
({ contents }) => {
23+
if (/async loadInstrumentationModule\(/.test(contents)) {
24+
return patchCode(contents, instrumentationRule);
25+
}
26+
}
27+
);
28+
}

‎packages/cloudflare/src/cli/build/patches/plugins/require-hook.ts‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
import { join } from "node:path";
2+
3+
import type { BuildOptions } from "@opennextjs/aws/build/helper.js";
4+
import { getCrossPlatformPathRegex } from "@opennextjs/aws/utils/regex.js";
5+
import type { Plugin } from "esbuild";
6+
7+
export function shimRequireHook(options: BuildOptions): Plugin {
8+
return {
9+
name: "replaceRelative",
10+
setup(build) {
11+
// Note: we (empty) shim require-hook modules as they generate problematic code that uses requires
12+
build.onResolve(
13+
{ filter: getCrossPlatformPathRegex(String.raw`^\./require-hook$`, { escape: false }) },
14+
() => ({
15+
path: join(options.outputDir, "cloudflare-templates/shims/empty.js"),
16+
})
17+
);
18+
},
19+
};
20+
}

‎packages/cloudflare/src/cli/build/patches/plugins/require-page.ts‎

Lines changed: 12 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -3,28 +3,22 @@ import { join } from "node:path";
33

44
import { type BuildOptions, getPackagePath } from "@opennextjs/aws/build/helper.js";
55
import { getCrossPlatformPathRegex } from "@opennextjs/aws/utils/regex.js";
6-
import type { PluginBuild } from "esbuild";
76

87
import { patchCode, type RuleConfig } from "../ast/util.js";
8+
import type { ContentUpdater } from "./content-updater.js";
99

10-
export function inlineRequirePagePlugin(buildOpts: BuildOptions) {
11-
return {
12-
name: "inline-require-page",
13-
14-
setup: async (build: PluginBuild) => {
15-
build.onLoad(
16-
{
17-
filter: getCrossPlatformPathRegex(String.raw`/next/dist/server/require\.js$`, { escape: false }),
18-
},
19-
async ({ path }) => {
20-
const jsCode = await readFile(path, "utf8");
21-
if (/function requirePage\(/.test(jsCode)) {
22-
return { contents: patchCode(jsCode, await getRule(buildOpts)) };
23-
}
24-
}
25-
);
10+
export function inlineRequirePagePlugin(updater: ContentUpdater, buildOpts: BuildOptions) {
11+
return updater.updateContent(
12+
"inline-require-page",
13+
{
14+
filter: getCrossPlatformPathRegex(String.raw`/next/dist/server/require\.js$`, { escape: false }),
2615
},
27-
};
16+
async ({ contents }) => {
17+
if (/function requirePage\(/.test(contents)) {
18+
return patchCode(contents, await getRule(buildOpts));
19+
}
20+
}
21+
);
2822
}
2923

3024
async function getRule(buildOpts: BuildOptions) {

‎packages/cloudflare/src/cli/build/patches/plugins/require.ts‎

Lines changed: 40 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -1,54 +1,44 @@
1-
import fs from "node:fs/promises";
1+
import type { ContentUpdater } from "./content-updater";
22

3-
import type { PluginBuild } from "esbuild";
3+
export function fixRequire(updater: ContentUpdater) {
4+
return updater.updateContent("fix-require", { filter: /\.(js|mjs|cjs|jsx|ts|tsx)$/ }, ({ contents }) => {
5+
// `eval(...)` is not supported by workerd.
6+
contents = contents.replaceAll(`eval("require")`, "require");
47

5-
export function fixRequire() {
6-
return {
7-
name: "fix-require",
8+
// `@opentelemetry` has a few issues.
9+
//
10+
// Next.js has the following code in `next/dist/server/lib/trace/tracer.js`:
11+
//
12+
// try {
13+
// api = require('@opentelemetry/api');
14+
// } catch (err) {
15+
// api = require('next/dist/compiled/@opentelemetry/api');
16+
// }
17+
//
18+
// The intent is to allow users to install their own version of `@opentelemetry/api`.
19+
//
20+
// The problem is that even when users do not explicitely install `@opentelemetry/api`,
21+
// `require('@opentelemetry/api')` resolves to the package which is a dependency
22+
// of Next.
23+
//
24+
// The second problem is that when Next traces files, it would not copy the `api/build/esm`
25+
// folder (used by the `module` conditions in package.json) it would only copy `api/build/src`.
26+
// This could be solved by updating the next config:
27+
//
28+
// const nextConfig: NextConfig = {
29+
// // ...
30+
// outputFileTracingIncludes: {
31+
// "*": ["./node_modules/@opentelemetry/api/build/**/*"],
32+
// },
33+
// };
34+
//
35+
// We can consider doing that when we want to enable users to install their own version
36+
// of `@opentelemetry/api`. For now we simply use the pre-compiled version.
37+
contents = contents.replace(
38+
/require\(.@opentelemetry\/api.\)/g,
39+
`require("next/dist/compiled/@opentelemetry/api")`
40+
);
841

9-
setup: async (build: PluginBuild) => {
10-
build.onLoad({ filter: /\.(js|mjs|cjs|jsx|ts|tsx)$/ }, async ({ path }) => {
11-
let contents = await fs.readFile(path, "utf-8");
12-
13-
// `eval(...)` is not supported by workerd.
14-
contents = contents.replaceAll(`eval("require")`, "require");
15-
16-
// `@opentelemetry` has a few issues.
17-
//
18-
// Next.js has the following code in `next/dist/server/lib/trace/tracer.js`:
19-
//
20-
// try {
21-
// api = require('@opentelemetry/api');
22-
// } catch (err) {
23-
// api = require('next/dist/compiled/@opentelemetry/api');
24-
// }
25-
//
26-
// The intent is to allow users to install their own version of `@opentelemetry/api`.
27-
//
28-
// The problem is that even when users do not explicitely install `@opentelemetry/api`,
29-
// `require('@opentelemetry/api')` resolves to the package which is a dependency
30-
// of Next.
31-
//
32-
// The second problem is that when Next traces files, it would not copy the `api/build/esm`
33-
// folder (used by the `module` conditions in package.json) it would only copy `api/build/src`.
34-
// This could be solved by updating the next config:
35-
//
36-
// const nextConfig: NextConfig = {
37-
// // ...
38-
// outputFileTracingIncludes: {
39-
// "*": ["./node_modules/@opentelemetry/api/build/**/*"],
40-
// },
41-
// };
42-
//
43-
// We can consider doing that when we want to enable users to install their own version
44-
// of `@opentelemetry/api`. For now we simply use the pre-compiled version.
45-
contents = contents.replace(
46-
/require\(.@opentelemetry\/api.\)/g,
47-
`require("next/dist/compiled/@opentelemetry/api")`
48-
);
49-
50-
return { contents };
51-
});
52-
},
53-
};
42+
return contents;
43+
});
5444
}

‎packages/cloudflare/src/cli/build/patches/to-investigate/index.ts‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,5 +2,4 @@ export * from "./inline-eval-manifest.js";
22
export * from "./inline-middleware-manifest-require.js";
33
export * from "./patch-exception-bubbling.js";
44
export * from "./patch-find-dir.js";
5-
export * from "./patch-load-instrumentation-module.js";
65
export * from "./patch-read-file.js";

0 commit comments

Comments
 (0)