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

Worker instantiation from URLs #30780

Closed
guybedford opened this issue Dec 3, 2019 · 7 comments
Closed

Worker instantiation from URLs #30780

guybedford opened this issue Dec 3, 2019 · 7 comments
Labels
esm Issues and PRs related to the ECMAScript Modules implementation. feature request Issues that request new features to be added to Node.js. worker Issues and PRs related to Worker support.

Comments

@guybedford
Copy link
Contributor

When creating workers from within modules, we don't have __filename or __dirname so rather have to rely on import.meta.url, something like:

import { Worker } from 'worker_threads';
import { fileURLToPath } from 'url';
new Worker(fileURLToPath(import.meta.url));

To avoid the fileURLToPath call being necessary here, should we consider supporting URLs as input into new Worker?

@guybedford guybedford added worker Issues and PRs related to Worker support. esm Issues and PRs related to the ECMAScript Modules implementation. labels Dec 3, 2019
@devsnek
Copy link
Member

devsnek commented Dec 3, 2019

IIRC we have prior art on accepting file urls for various APIs, but i don't remember which ones.

@addaleax
Copy link
Member

addaleax commented Dec 3, 2019

IIRC we have prior art on accepting file urls for various APIs, but i don't remember which ones.

The initial URL support for the fs module supported this, but it was later withdrawn from the PR because a) it could invalidate previously existing security checks when reading from paths that are partially user-controlled and b) file URLs are, sadly, also valid paths for files in directories named file:.

I think a) is not a large concern here.

@guybedford
Copy link
Contributor Author

b) has typically been also seen as a backwards-incompatible change. Perhaps to implement it that way upfront now though would be permissible.

Then, if users really need a directory named file: they can convert the path into a URL.

@coreyfarrell
Copy link
Member

coreyfarrell commented Dec 5, 2019

Support for new Worker(import.meta) would eliminate ambiguity. Just a thought, not suggesting it's a good idea.

That said a filename matching /^file:/ or /^data:/ is very edge case for this API, does anyone even use new Worker(pathRelativeToCWD)? The documentation only suggests new Worker(__filename) which would never result in a filename that is ambiguous to URL's. new Worker('./other-file.js') would be unreliable as it would be based on process.cwd(), instead new Worker(require.resolve('./other-file.js')) or new Worker(new URL('./other-file.js', import.meta.url).href) would be needed. I'd go as far as saying that passing a relative path to this API should be an error.

@targos
Copy link
Member

targos commented Dec 5, 2019

does anyone even use new Worker(pathRelativeToCWD) ?

I used it multiple times. It is convenient in apps to always use new Worker('./src/workers/myworker.js') from anywhere in the hierarchy and as not necessarily unreliable. My apps are always started with their root as working directory.

@jasnell
Copy link
Member

jasnell commented Dec 5, 2019

Just having something like the following would work...

new Worker(new URL(import.meta.url));

@Jamesernator
Copy link

^ This approach is how fs accepts URLs, it just accepts URL objects, strings are always treated as paths.

aduh95 added a commit to aduh95/node that referenced this issue Mar 11, 2020
The explicit goal is to let users use `import.meta.url` to re-load thecurrent module inside a Worker instance.

Fixes: nodejs#30780
@guybedford guybedford added the feature request Issues that request new features to be added to Node.js. label Mar 12, 2020
BridgeAR pushed a commit that referenced this issue Mar 17, 2020
The explicit goal is to let users use `import.meta.url` to
re-load thecurrent module inside a Worker instance.

Fixes: #30780
PR-URL: #31664
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
MylesBorins pushed a commit that referenced this issue Mar 24, 2020
The explicit goal is to let users use `import.meta.url` to
re-load thecurrent module inside a Worker instance.

Fixes: #30780
PR-URL: #31664
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
targos pushed a commit to targos/node that referenced this issue Apr 25, 2020
The explicit goal is to let users use `import.meta.url` to
re-load thecurrent module inside a Worker instance.

Fixes: nodejs#30780
PR-URL: nodejs#31664
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
targos pushed a commit that referenced this issue Apr 28, 2020
The explicit goal is to let users use `import.meta.url` to
re-load thecurrent module inside a Worker instance.

Fixes: #30780
PR-URL: #31664
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
esm Issues and PRs related to the ECMAScript Modules implementation. feature request Issues that request new features to be added to Node.js. worker Issues and PRs related to Worker support.
Projects
None yet
Development

No branches or pull requests

7 participants