cloudflare / workers-sdk

⛅️ Home to Wrangler, the CLI for Cloudflare Workers®
https://developers.cloudflare.com/workers/
Apache License 2.0
2.59k stars 673 forks source link

Upgrade `esbuild` to `0.19.0`+ #5222

Open mrbbot opened 6 months ago

mrbbot commented 6 months ago

Following on from #5213, this issue tracks upgrading the esbuild version of all tools in workers-sdk including wrangler to 0.19.0 and above. 0.19.0 adds support for bundling import paths containing wildcards (e.g. await import("./data/${lang}.json") will bundle all files matching ./data/**/*.json, then select the correct module at runtime). Wrangler currently allows module rules to support these kinds of variable-based dynamic imports. Wildcard imports cannot be disabled, and do not trigger onResolve() plugin hooks (if they did, we could mark them as external effectively disabling this feature).

Potential paths forward

Option 1: integrate wildcard imports with custom module rules

In Wrangler's module collector, we would continue to mark modules matching module rules as external. Whilst modules imported via wildcard imports don't trigger onResolve() plugin hooks, they do trigger onLoad() hooks for each matched module. If an onLoad() hook was triggered, we'd know the module must've come from a wildcard import as the onResolve() hook marks collected modules as external, and externals don't trigger onLoad(). We could add an onLoad() hook matching module rules that re-exported the module from an external import(). We'd have to be very careful here not to convert dynamic imports into static imports (i.e. we don't want await import(...) to become multiple import ... from "..."s). Users rely on dynamic imports to lazy-load parts of their code and we don't want to prevent this.

Even without additional module rules, the wildcard import feature allows files esbuild natively understands to be bundled. For example, await import("./data/${lang}.js") would be bundled without any additional configuration from the user. With our current esbuild bundle, this would result in a runtime error as the module wouldn't be included in the Worker upload. With this in mind, the wildcard import feature is providing functionality that wasn't previously supported. However, there is a danger it bundles too many files (e.g. node_modules or files that would cause a build time error because of unsupported node:* modules). Workers have a strict size limit, and wildcard imports could easily create bundles exceeding this.

If we enabled this feature, we'd likely want to inform affected users. Currently, esbuild generates output that looks like this for wildcard imports:

// import("./data/**/*.txt") in src/index.mjs
var globImport_data_txt = __glob({
  "./data/a.txt": () => Promise.resolve().then(() => ...),
  "./data/b.txt": () => Promise.resolve().then(() => ...),
  "./data/c.txt": () => Promise.resolve().then(() => ...)
});

// src/index.mjs
var module = await globImport_data_txt(`./data/${name}.txt`);

We could regexp on // import("...") in ... \n __glob( in the output to warn users when a wildcard import was bundled.

Option 2: mark modules matched by custom module rules as top-level external

Instead of implementing the module collector as an esbuild plugin, we could run it before esbuild, then pass its collected modules to the top-level external option. Whilst wildcard imports don't trigger onResolve() hooks, they do respect this option. For example, setting external: ["*.wasm"], then including await import("./data/*.wasm") in source code results in the following output:

// import("./data/**/*.wasm") in src/index.mjs
var globImport_data_wasm = __glob({
  "./data/a.wasm": () => import("./data/a.wasm"),
  "./data/b.wasm": () => import("./data/b.wasm")
  "./data/c.wasm": () => import("./data/c.wasm")
});

// src/index.mjs
var module = await globImport_data_wasm(`./data/${name}.wasm`);

If we did this, we'd need to restart esbuild whenever new modules matching rules were added. This option still has a danger of bundling too many files like Option 1.

Option 3: submit PR to esbuild to allow this feature to controlled

Ideally, a mechanism would be added to esbuild to disable wildcard imports. This could either be via a top-level build option or via a plugin hook. A build option would be relatively simple to implement, but having the ability to control this feature via a plugin would be even better. There are a few ways this could be supported:

  1. Call onResolve() with the glob pattern generated from the template literal/string concatenation as args.path, and allow onResolve() to return multiple module paths in this case. If the onResolve() hook returned { external: true }, the import() statement would be bundled verbatim. This would also allow wildcard imports to be supported by esbuild running in the browser, which relies on plugins to provide a virtual "file system".
  2. Add a new onGlobResolve() hook with similar behaviour to the above.
  3. Call onResolve() with each module matching the glob pattern. Bundling wildcard imports can be thought of as a three step process: first the import is converted to a glob pattern, second the pattern is evaluated and becomes __glob({ ...: () => import(...), ... }), third the new __glob() code is bundled as usual. This intermediate second stage can be seen when using the top-level external option as in Option 2. Arguably, onResolve() should be called on each of these intermediate import(...) statements, which would allow us to mark them as external.

Option 4: consider switching to another bundler

If none of these options work, we could consider another more-flexible bundler like Rollup or the newer Rolldown when that matures. We could also consider switching to a "bundle-less" dev approach like Vite, potentially using the module-fallback service in a similar way to the new Vitest integration. This would move away from wrangler dev running pretty much the same code that gets deployed, which is a nice guarantee to have (though arguably already broken by the middleware system).

penalosa commented 2 months ago

@threepointone

aaronadamsCA commented 2 months ago

Hopefully it's appropriate to start noting real-world problems this is starting to cause.

@shopify/shopify-app-remix needs a patch to run on Wrangler because it contains an import attribute: https://github.com/Shopify/shopify-app-js/issues/1031

Cherry commented 3 weeks ago

Resurfacing from https://github.com/cloudflare/workers-sdk/issues/2980

wrangler is using 0.17.19, released over a year ago in May 2023.

I hit this again today when trying to do something like this:

import package from '../package.json' with { type: 'json' }

wrangler throws:

✘ [ERROR] Expected ";" but found "with"

    src/worker.ts:1:35:
      1 │ import json from '../package.json' with { type: 'json' };
        │                                    ~~~~

from the old esbuild version.

But manually bundling with even just version 0.1912: npx esbuild@0.19.12 src/worker.ts --bundle --outfile=dist.js --target=esnext --format=esm --platform=neutral

works fine.

threepointone commented 2 weeks ago

The workaround (for now) is to use overrides in your package.json. For example, if you're using npm:

// ...
  "overrides": {
    "esbuild": "0.21.4"
  },
// ...