Skip to content
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

Add type option to Worker constructor #31760

Closed

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Feb 12, 2020

This PR adds a type option to the Worker constructor, inspired from WorkerType in HTML spec, and support for data: URLs.

  • When eval is true OR filename is a data: URL object, the type option explicitly tells Node.js what kind of JS to expect (ESM or CJS).
  • Otherwise, the option is ignored and the usual file extension rules is used.
// use ESM loader to load the worker.js file
new Worker('./worker.js', { type: 'module' });

// eval ESM
new Worker('export default import.meta.url', { eval: true, type: 'module' });

// data: URL
new Worker(
  new URL('data:text/javascript,export default import.meta.url'),
  { type: 'module' }
);

Note: new Worker('./worker.cjs', {type: 'module'}) would load worker.cjs as ES module and new Worker('./worker.mjs', {type: 'classic'}) would load worker.mjs as 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. This has been reverted based on this PR's comments.

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), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Feb 12, 2020
@aduh95
Copy link
Contributor Author

aduh95 commented Feb 13, 2020

Fixes #30682 btw

@MylesBorins MylesBorins added esm Issues and PRs related to the ECMAScript Modules implementation. modules-agenda Issues and PRs to discuss during the meetings of the Modules team. labels Feb 13, 2020
@MylesBorins
Copy link
Contributor

This seems reasonable to me. I've added to the modules agenda for visibility

@guybedford
Copy link
Contributor

guybedford commented Feb 14, 2020

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.

@guybedford
Copy link
Contributor

Also I'm wondering if worker evaluation might be achieved with data URIs?

@aduh95
Copy link
Contributor Author

aduh95 commented Feb 14, 2020

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.

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 type option, and give users a way to shortcut the extension checking.

@aduh95 aduh95 force-pushed the add-support-for-extensionless-workers branch from 6493dff to 27ee5a9 Compare February 15, 2020 22:10
@aduh95
Copy link
Contributor Author

aduh95 commented Feb 15, 2020

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.

@jkrems
Copy link
Contributor

jkrems commented Feb 24, 2020

Note: new Worker('./worker.cjs', {type: 'module'}) would load worker.cjs as ES module and new Worker('./worker.mjs', {type: 'classic'}) would load worker.mjs as CJS.

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, type: 'module' wouldn't run a resource with content-type: application/node as a JavaScript module.

I think the type option still makes sense to chose the module system (require by default, import when type is set to module). But I don't think it should affect content type resolution.

@bmeck
Copy link
Member

bmeck commented Feb 25, 2020

We could use a type assertion instead, which seems to be the difference of as (which asserts the type of a target from link tag / module attributes proposal) and type (which changes the loading system of a target).

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 type: 'wasm' in package.json it likely would be argument against using a type field in the constructor here as well.

Since the ESM loader does support data:, and "bin":{"foo":"bin/foo.cjs"} within package.json can point to a file with an extension it might also just be prudent to not do anything except having a switch to select the loader used for the initial worker endpoint; this is similar to how shouldUseESMLoader works

function shouldUseESMLoader(mainPath) {
in a dynamic manner and wouldn't appear to cause any new forms of conflicts where resources can be interpreted in multiple different ways.

@MylesBorins MylesBorins removed the modules-agenda Issues and PRs to discuss during the meetings of the Modules team. label Feb 26, 2020
@aduh95 aduh95 force-pushed the add-support-for-extensionless-workers branch from 27ee5a9 to 19cb211 Compare March 6, 2020 21:47
@aduh95 aduh95 changed the title Add support for extensionless workers and ESM eval Add type option to Worker constructor Mar 6, 2020
@addaleax addaleax added semver-minor PRs that contain new features and should be released in the next minor version. worker Issues and PRs related to Worker support. and removed lib / src Issues and PRs related to general changes in the lib or src directory. labels Mar 29, 2020
@addaleax
Copy link
Member

@aduh95 This looks like it needs to be rebased.

@nodejs/modules-active-members Should this go ahead? It seems good to me.

@bmeck
Copy link
Member

bmeck commented Mar 29, 2020

We likely should not land this yet. Discussion of expanding to handle the non-JS cases isn't ironed out.

@aduh95 aduh95 force-pushed the add-support-for-extensionless-workers branch 2 times, most recently from a783efa to f66e0b3 Compare March 29, 2020 23:43
@GeoffreyBooth GeoffreyBooth added the blocked PRs that are blocked by other issues or PRs. label Mar 30, 2020
@jkrems
Copy link
Contributor

jkrems commented Mar 30, 2020

Should this option be called inputType since it's roughly equivalent to --input-type on the command line?

@aduh95
Copy link
Contributor Author

aduh95 commented Apr 4, 2020

Should this option be called inputType since it's roughly equivalent to --input-type on the command line?

Should it be restricted to eval: true like the CLI option?

evalModule now returns a Promise to let implementors handle errors.
Introduces a new evalModuleOrCrash function that implement the crashing
behaviour.
@aduh95 aduh95 force-pushed the add-support-for-extensionless-workers branch from f66e0b3 to 109bc0e Compare May 30, 2020 08:36
@aduh95
Copy link
Contributor Author

aduh95 commented May 30, 2020

I have removed the loader selection for this PR to unblock it.

I have also added support for data: URLs as #31760 (comment) suggested.

@aduh95 aduh95 force-pushed the add-support-for-extensionless-workers branch from 109bc0e to 2e5f780 Compare May 30, 2020 08:55
@BridgeAR BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:19
@aduh95
Copy link
Contributor Author

aduh95 commented Aug 19, 2020

Closing this in favor of #34584.

@aduh95 aduh95 closed this Aug 19, 2020
@aduh95 aduh95 deleted the add-support-for-extensionless-workers branch August 19, 2020 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked PRs that are blocked by other issues or PRs. esm Issues and PRs related to the ECMAScript Modules implementation. semver-minor PRs that contain new features and should be released in the next minor version. worker Issues and PRs related to Worker support.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Worker eval as ES Module
8 participants