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

Motivation for inferring that a given worker is a module worker #7

Open
codehag opened this issue Apr 3, 2024 · 4 comments
Open

Motivation for inferring that a given worker is a module worker #7

codehag opened this issue Apr 3, 2024 · 4 comments

Comments

@codehag
Copy link

codehag commented Apr 3, 2024

This is a minor thing, but one section of the readme points out that {type: module} will be inferred:

import source myModule from "./my-module.js";

// `{ type: 'module' }` can be inferred since myModule is a module object
const worker = new Worker(myModule);

This seems bad for readability and adds additional overhead to the act of reading. It reduces typing time, but reading time is more significant. I lean towards keeping this explicit. Do we have other reasons why this would be inferred?

@guybedford
Copy link
Collaborator

I hadn't thought about the readability benefit here actually. What aspect of that do you feel is important to maintain?

From a tooling perspective there is some minor non-analyzability concerns where new Worker(myModule, options) might not be able to know the contents of options via static analysis. So a build tool might not know whether to build the worker as a script or a module.

I suppose the tool can already infer that the options should be setting type: 'module' because otherwise the worker will throw an error that modules cannot be run in script workers, so the tool could assume the correct execution as opposed to failure.

Forgetting to add type: 'module' is effectively a footgun (albeit a minor one), and I do have a preference for enabling the ergonomic instantiation without it being necessary personally. new Worker(myModule, { type: 'module' } would still be supported so it would only be a change of default.

@codehag
Copy link
Author

codehag commented Apr 4, 2024

At the moment, as a reader, if you don't see an options object passed you can assume it is a classic worker. This change will result in having to also check how the url loaded by the module is defined. In addition, this is implicit knowledge, so for new developers looking at documentation, they may not realize that there is this second implicit meaning of new Worker(myModule). This also has implications for search, where the context of the file is not shown.

This isn't a critical aspect, but I would like this to be thought through if we change this. It shouldn't be justified from the perspective that it is more ergonomic to write the code. Writing is a less frequent action than reading.

@nicolo-ribaudo
Copy link
Member

From a tooling perspective there is some minor non-analyzability concerns where new Worker(myModule, options) might not be able to know the contents of options via static analysis. So a build tool might not know whether to build the worker as a script or a module.

If we don't want to infer { type: "module" }, we should explicitly throw when creating a worker using a module source and without setting the type (and absolutely do not try to reinterpret the module as a script). So tools can just assume that options contains type: "module" when the first argument is a module source.

@guybedford
Copy link
Collaborator

I agree with Nicolo that having an early error for attempting to load the worker as a script would be worthwhile.

With that, I'd be open to considering removing the inference if the arguments for readability are greater than the static analysis ones here.

The only other static analysis risk I can think of is cases where users write new Worker(mod, module) where they separately define module = { type: "module" } above. Tools won't be able to know if there are other worker options being provided or not, say for example in future we support an importMap option. Where not knowing if a custom import map is being provided or not causes a resolution deoptimization or something like that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants