Add type option to Worker constructor#31760
Conversation
|
Fixes #30682 btw |
|
This seems reasonable to me. I've added to the modules agenda for visibility |
|
It would help to understand a driving use case for needing this feature (by that I mean exensionless workers). We don't have a problem with the fact that extensionless files cannot be imported from ES modules, so I'm just wondering why workers should be different. |
|
Also I'm wondering if worker evaluation might be achieved with data URIs? |
Maybe should split this PR so we can discuss the two issues separately? My thinking was to make Node.js worker implementation closer to the HTML spec when using |
6493dff to
27ee5a9
Compare
|
After digging a bit more, it turns out the implementation is sloppier than I first imagined. I'll be waiting on modules team feedback if we need/want the actual feature before putting more work into it. If we want to move this forward, I think I could benefit a lot from the changes on #31229, so maybe wait for it to land, then implement the feature requested on #21667, which could be used for a cleaner implementation. |
That sounds like this circumvents the type detection entirely which isn't compatible with what the web equivalent does. Think of the file extension as our de-facto content-type header. On the web, I think the type option still makes sense to chose the module system ( |
|
We could use a type assertion instead, which seems to be the difference of However, per things like #31388 which was brought about by extension-less support; we probably want to think about how this would expand in the future for formats outside of JS itself. I think if people were uncomfortable with Since the ESM loader does support node/lib/internal/modules/run_main.js Line 23 in f5ef7cd |
27ee5a9 to
19cb211
Compare
|
@aduh95 This looks like it needs to be rebased. @nodejs/modules-active-members Should this go ahead? It seems good to me. |
|
We likely should not land this yet. Discussion of expanding to handle the non-JS cases isn't ironed out. |
a783efa to
f66e0b3
Compare
|
Should this option be called |
Should it be restricted to |
evalModule now returns a Promise to let implementors handle errors. Introduces a new evalModuleOrCrash function that implement the crashing behaviour.
Allows eval of ESM string. Fixes: nodejs#30682
f66e0b3 to
109bc0e
Compare
|
I have removed the loader selection for this PR to unblock it. I have also added support for |
109bc0e to
2e5f780
Compare
8ae28ff to
2935f72
Compare
|
Closing this in favor of #34584. |
This PR adds a
typeoption to theWorkerconstructor, inspired from WorkerType in HTML spec, and support fordata:URLs.evalis true ORfilenameis adata:URL object, thetypeoption explicitly tells Node.js what kind of JS to expect (ESM or CJS).Note:This has been reverted based on this PR's comments.new Worker('./worker.cjs', {type: 'module'})would loadworker.cjsas ES module andnew Worker('./worker.mjs', {type: 'classic'})would loadworker.mjsas CJS. I don't expect anyone to have a use-case for this edge-case, but I wanted to clarify it's a possibility with this PR's implementation.I had to tweak some internal functions, I have put those changes in separate commits to make reviewing them easier.
Fixes: #30682
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes