Support modules with --no-bundle
#2769
Conversation
🦋 Changeset detectedLatest commit: b7e71a6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
A wrangler prerelease is available for testing. You can install this latest build in your project with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/4533143746/npm-package-wrangler-2769You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/2769/npm-package-wrangler-2769Or you can use npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/4533143746/npm-package-wrangler-2769 dev path/to/script.jsAdditional artifacts:npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/4533143746/npm-package-cloudflare-pages-shared-2769Note that these links will no longer work once the GitHub Actions artifact expires. |
penalosa
force-pushed
the
penalosa/no-bundle-modules
branch
from
da67a09
to
1d6a6e2
Compare
Codecov Report
@@ Coverage Diff @@
## main #2769 +/- ##
==========================================
+ Coverage 73.49% 74.01% +0.51%
==========================================
Files 166 168 +2
Lines 10427 10358 -69
Branches 2791 2761 -30
==========================================
+ Hits 7663 7666 +3
+ Misses 2764 2692 -72
|
There was a problem hiding this comment.
This looks awesome! I've left one comment, but overall I think this is looking great at a high level.
I'm also however not the most qualified to review this as I'm not deeply familiar with internals of Wrangler so I'll leave defer to someone on the team to do an in-depth review and approve this change.
But thank you for putting this together! I appreciate it.
| const resp = await worker.fetch("/dynamic-var"); | ||
| const text = await resp.text(); | ||
| expect(text).toMatchInlineSnapshot( | ||
| '"Error: No such module \\"dynamic-var.js\\"."' |
There was a problem hiding this comment.
is this the right error message? oh.. wait.. are we throwing this error because we failed to upload that file and then runtime couldn't find it, hence this error message?
would it be possible for the --no-bundle code to fail if we can't resolve dynamic imports? As a developer I'd rather see a build error than runtime error.
There was a problem hiding this comment.
Yeah, this gets all the way to the runtime. Unfortunately, esbuild doesn't provide any hooks for dynamic variable imports—it seems to just ignore them (see https://esbuild.github.io/api/#non-analyzable-imports), so I'm not sure there's much we can do here. This might be possible to support in future somehow with https://www.npmjs.com/package/es-module-lexer though
There was a problem hiding this comment.
If we can't find a way for --no-bundle to fail when dynamic imports can't be resolved, could this error message read something like:
"Error: The dynamically imported ES module \"dynamic-var.js"\ could not be found. Run wrangler --dry-run --outdir --no-bundle and verify that this module was generated when building your worker"
...such that we give people instructions on how to debug? (not sure if I've got the instructions quite right above, but you get the gist). How can we give people an error message they can act on and debug on their own?
There was a problem hiding this comment.
I'm not sure—this error comes from the runtime, so we may not want to call out Wrangler specifically. Maybe something like "Error: The dynamically imported ES module "dynamic-var.js"\ could not be found. Make sure it was uploaded when you published your Worker"?
There was a problem hiding this comment.
I was going to ask about [es-module-lexer](https://www.npmjs.com/package/es-module-lexer)... could you please do a lightweight exploration if implementing this feature via es-module-lexer would allow us to turn the runtime error into build time error?
If that was possible and there were no other major blockers, I'd be inclined to going down that path as runtime errors are really really nasty and we should avoid them whenever we can even if it means a little more work for us — it will save many times more work to developers that would otherwise scramble to deal with runtime errors.
There was a problem hiding this comment.
I've pushed up an additional commit that adds a build-time error for non string-literal dynamic imports. It uses es-module-lexer in addition to esbuild, and so has a slight performance cost over just using esbuild (but that should be minimal). They're both needed in order to be able to run Wrangler's module collection (which is implemented as an esbuild plugin), and to avoid re-implementing esbuild's module resolution (reading tsconfig, for instance)
There was a problem hiding this comment.
I see. Interesting. How significant is the overhead? Do you have any estimates? <5ms for an average project, or more? Unless it's overhead measured in hundreds of ms, I think it's totally worth it. I am hoping for single or at most double digit ms...
There was a problem hiding this comment.
It's pretty minimal. es-module-lexer has benchmarks which estimate 5-10ms as an upper bound for parsing a MB of JS, but it will scale with the number of modules. For a large worker (let's say 25 modules at 200kb, which is at the 5MB limit for workers), es-module-lexer will probably add about 50ms of latency in the worst case (with a fairly pessimistic reading of their benchmarks).
There was a problem hiding this comment.
For an average project (let's say 5 modules at 100kb), the overhead would be closer to ~5ms
There was a problem hiding this comment.
Great. Then this overhead is totally worth it. Thanks
|
Oh, and also this needs a changeset! (and probably a small docs update?) |
packages/wrangler/package.json
Outdated
| @@ -105,7 +105,7 @@ | |||
| "@miniflare/durable-objects": "2.12.1", | |||
| "blake3-wasm": "^2.1.5", | |||
| "chokidar": "^3.5.3", | |||
| "esbuild": "0.16.3", | |||
| "esbuild": "0.16.5", | |||
There was a problem hiding this comment.
Was the "external" feature from ESBuild needed for the feature to be complete? Just trying to understand the ESBuild bump and "external" feature interaction with the --no-bundle feature 😁
There was a problem hiding this comment.
Yep, the external feature is needed. This PR uses esbuild to try and figure out which additional modules should be uploaded with your entrypoint file. Without packages: external we'd try and upload your package imports as well as just file imports. This is pretty much only relevant for node:* node-compat imports right now, but more might be added in future.
We could try and figure out which package imports could be safely uploaded, to support runtime importing of npm modules, but the right solution for that is bundling.
There was a problem hiding this comment.
Appreciate the clarification!! 🎆
There was a problem hiding this comment.
the right solution for that is bundling.
So like https://www.npmjs.com/package/rollup lol 😆
| ).map(([name, m]) => ({ name, ...m })); | ||
|
|
||
| if (collectedModules.length > 0) { | ||
| logger.info(`Detected data modules. Uploading additional data modules:`); |
There was a problem hiding this comment.
Would the user understand what "data modules" means here? I am not sure I do. From looking at the code, I think they are modules that matched the wrangler.toml "rules" for including imported files?
There was a problem hiding this comment.
How about "Detected non-javascript module rules. Uploading additional modules:"?
|
@petebacondarwin CI should be green now, if you have a chance to take another look! |
What this PR solves / how to test:
When the
--no-bundleflag is set, this PR adds support for traversing the module graph withesbuild, to figure out what additional modules should be uploaded alongside the entrypoint to support runtime imports. It also bumpsesbuildto 16.5 to supportpackages: external.Associated docs issues/PR:
Author has included the following, where applicable:
Reviewer has performed the following, where applicable:
Fixes #2697.