Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support modules with
--no-bundle
#2769Support modules with
--no-bundle
#2769Changes from 11 commits
84c9e71
9899192
8b67243
1d6a6e2
1be1903
b577735
ab9ea6e
b925ec8
2f7fc20
5be3df0
58b6a0f
b268495
b1a85c7
ab50e01
d715f59
bf59ecf
eb16a46
e70b587
80896d2
d7a6aba
be45c79
36666cd
6bbe79b
c0bc8f3
203e212
88e98c4
b7e71a6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT - I feel "lazy" is more helpful here than "dynamic" even though I appreciate that this is the official term for an
import()
expression like this. The reason is that there is a qualitative difference betweenawait import("some-module")
andlet x = "some-module"; await import(x)
, since the former can be analysed statically by esbuild and the latter cannot.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the same bug preventing you from just leaving this undefined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 thoughThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 toesbuild
, and so has a slight performance cost over just usingesbuild
(but that should be minimal). They're both needed in order to be able to run Wrangler's module collection (which is implemented as anesbuild
plugin), and to avoid re-implementingesbuild
's module resolution (readingtsconfig
, for instance)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great. Then this overhead is totally worth it. Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: to space or not to space (out the
test()
calls) that is the question.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this
no-bundle-import
fixture could benefit from a README that explains what it is exercising.