refactor(ContentUpdater): add a filterContent argument (#365)

It is used as a fast heuristics to trigger the update

Author: vicb

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

@@ -8,11 +8,21 @@ import { readFile } from "node:fs/promises";
 
 import { type OnLoadArgs, type OnLoadOptions, type Plugin, type PluginBuild } from "esbuild";
 
+/**
+ * The callbacks returns either an updated content or undefined if the content is unchanged.
+ */
 export type Callback = (args: {
   contents: string;
   path: string;
 }) => string | undefined | Promise<string | undefined>;
-export type Updater = OnLoadOptions & { callback: Callback };
+
+/**
+ * The callback is called only when `contentFilter` matches the content.
+ * It can be used as a fast heuristic to prevent an expensive update.
+ */
+export type OnUpdateOptions = OnLoadOptions & { contentFilter: RegExp };
+
+export type Updater = OnUpdateOptions & { callback: Callback };
 
 export class ContentUpdater {
   updaters = new Map<string, Updater>();
@@ -23,11 +33,11 @@ export class ContentUpdater {
    * The callbacks are called in order of registration.
    *
    * @param name The name of the plugin (must be unique).
-   * @param options Same options as the `onLoad` hook to restrict updates.
+   * @param options Options.
    * @param callback The callback updating the content.
    * @returns A noop ESBuild plugin.
    */
-  updateContent(name: string, options: OnLoadOptions, callback: Callback): Plugin {
+  updateContent(name: string, options: OnUpdateOptions, callback: Callback): Plugin {
     if (this.updaters.has(name)) {
       throw new Error(`Plugin "${name}" already registered`);
     }
@@ -48,13 +58,16 @@ export class ContentUpdater {
       setup: async (build: PluginBuild) => {
         build.onLoad({ filter: /\.(js|mjs|cjs|jsx|ts|tsx)$/ }, async (args: OnLoadArgs) => {
           let contents = await readFile(args.path, "utf-8");
-          for (const { filter, namespace, callback } of this.updaters.values()) {
+          for (const { filter, namespace, contentFilter, callback } of this.updaters.values()) {
             if (namespace !== undefined && args.namespace !== namespace) {
               continue;
             }
             if (!filter.test(args.path)) {
               continue;
             }
+            if (!contentFilter.test(contents)) {
+              continue;
+            }
             contents = (await callback({ contents, path: args.path })) ?? contents;
           }
           return { contents };

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

@@ -18,11 +18,7 @@ fix: async loadInstrumentationModule() { }
 export function patchLoadInstrumentation(updater: ContentUpdater) {
   return updater.updateContent(
     "patch-load-instrumentation",
-    { filter: /\.(js|mjs|cjs|jsx|ts|tsx)$/ },
-    ({ contents }) => {
-      if (/async loadInstrumentationModule\(/.test(contents)) {
-        return patchCode(contents, instrumentationRule);
-      }
-    }
+    { filter: /\.(js|mjs|cjs|jsx|ts|tsx)$/, contentFilter: /async loadInstrumentationModule\(/ },
+    ({ contents }) => patchCode(contents, instrumentationRule)
   );
 }

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

@@ -12,12 +12,9 @@ export function inlineRequirePagePlugin(updater: ContentUpdater, buildOpts: Buil
     "inline-require-page",
     {
       filter: getCrossPlatformPathRegex(String.raw`/next/dist/server/require\.js$`, { escape: false }),
+      contentFilter: /function requirePage\(/,
     },
-    async ({ contents }) => {
-      if (/function requirePage\(/.test(contents)) {
-        return patchCode(contents, await getRule(buildOpts));
-      }
-    }
+    async ({ contents }) => patchCode(contents, await getRule(buildOpts))
   );
 }
 

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

@@ -1,44 +1,48 @@
 import type { ContentUpdater } from "./content-updater";
 
 export function fixRequire(updater: ContentUpdater) {
-  return updater.updateContent("fix-require", { filter: /\.(js|mjs|cjs|jsx|ts|tsx)$/ }, ({ contents }) => {
-    // `eval(...)` is not supported by workerd.
-    contents = contents.replaceAll(`eval("require")`, "require");
+  return updater.updateContent(
+    "fix-require",
+    { filter: /\.(js|mjs|cjs|jsx|ts|tsx)$/, contentFilter: /.*/ },
+    ({ contents }) => {
+      // `eval(...)` is not supported by workerd.
+      contents = contents.replaceAll(`eval("require")`, "require");
 
-    // `@opentelemetry` has a few issues.
-    //
-    // Next.js has the following code in `next/dist/server/lib/trace/tracer.js`:
-    //
-    //     try {
-    //       api = require('@opentelemetry/api');
-    //     } catch (err) {
-    //       api = require('next/dist/compiled/@opentelemetry/api');
-    //     }
-    //
-    // The intent is to allow users to install their own version of `@opentelemetry/api`.
-    //
-    // The problem is that even when users do not explicitely install `@opentelemetry/api`,
-    // `require('@opentelemetry/api')` resolves to the package which is a dependency
-    // of Next.
-    //
-    // The second problem is that when Next traces files, it would not copy the `api/build/esm`
-    // folder (used by the `module` conditions in package.json) it would only copy `api/build/src`.
-    // This could be solved by updating the next config:
-    //
-    //     const nextConfig: NextConfig = {
-    //       // ...
-    //       outputFileTracingIncludes: {
-    //         "*": ["./node_modules/@opentelemetry/api/build/**/*"],
-    //       },
-    //     };
-    //
-    // We can consider doing that when we want to enable users to install their own version
-    // of `@opentelemetry/api`. For now we simply use the pre-compiled version.
-    contents = contents.replace(
-      /require\(.@opentelemetry\/api.\)/g,
-      `require("next/dist/compiled/@opentelemetry/api")`
-    );
+      // `@opentelemetry` has a few issues.
+      //
+      // Next.js has the following code in `next/dist/server/lib/trace/tracer.js`:
+      //
+      //     try {
+      //       api = require('@opentelemetry/api');
+      //     } catch (err) {
+      //       api = require('next/dist/compiled/@opentelemetry/api');
+      //     }
+      //
+      // The intent is to allow users to install their own version of `@opentelemetry/api`.
+      //
+      // The problem is that even when users do not explicitly install `@opentelemetry/api`,
+      // `require('@opentelemetry/api')` resolves to the package which is a dependency
+      // of Next.
+      //
+      // The second problem is that when Next traces files, it would not copy the `api/build/esm`
+      // folder (used by the `module` conditions in package.json) it would only copy `api/build/src`.
+      // This could be solved by updating the next config:
+      //
+      //     const nextConfig: NextConfig = {
+      //       // ...
+      //       outputFileTracingIncludes: {
+      //         "*": ["./node_modules/@opentelemetry/api/build/**/*"],
+      //       },
+      //     };
+      //
+      // We can consider doing that when we want to enable users to install their own version
+      // of `@opentelemetry/api`. For now we simply use the pre-compiled version.
+      contents = contents.replace(
+        /require\(.@opentelemetry\/api.\)/g,
+        `require("next/dist/compiled/@opentelemetry/api")`
+      );
 
-    return contents;
-  });
+      return contents;
+    }
+  );
 }