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.

Sign up for GitHub

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

Jump to bottom

worker_threads: allow URL in Worker constructor #31664

Closed
wants to merge 2 commits into from
Closed

worker_threads: allow URL in Worker constructor #31664

wants to merge 2 commits into from

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Feb 6, 2020

The explicit goal is to let users use import.meta.url to re-load the
current module inside a Worker instance.

I wasn't sure where to put the test file, I've put it next to the other worker tests, but it's the only .mjs file in this folder so it may be odd.

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 worker Issues and PRs related to Worker support. label Feb 6, 2020
@addaleax
Copy link
Member

addaleax commented Feb 6, 2020

@nodejs/workers This is technically a breaking change, but I could see that we would allow it as a semver-minor addition as it’s highly unlikely that file:/// URLs are currently being passed as strings to the Worker constructor. Any thoughts?

jasnell
jasnell requested changes Feb 6, 2020
View reviewed changes
doc/api/worker_threads.md Show resolved Hide resolved
lib/internal/worker.js Outdated Show resolved Hide resolved
test/parallel/test-worker.mjs Outdated Show resolved Hide resolved
@jasnell
Copy link
Member

jasnell commented Feb 6, 2020

I'm good with this as a semver-minor and not semver-major. Workers are so new that I would be extremely surprised if anything breaks and the use case of new Worker(import.meta.url) is likely to be common enough that this is worthwhile.

@jasnell jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Feb 6, 2020
@aduh95 aduh95 force-pushed the add-support-for-url-in-worker-constructor branch from ac77462 to b65b8a0 Compare February 6, 2020 19:56
@aduh95
Copy link
Contributor Author

aduh95 commented Feb 6, 2020

I'm dropping support for file:// URL strings, this PR only adds support for WHATWG URL objects to address @addaleax comments. It makes Node.js behaviour consistent between fs and worker_threads, so that's probably better.
new Worker(import.meta.url) doesn't work anymore, but new Worker(new URL(import.meta.url)) is close enough I'd say.

I have added some more test cases, as @jasnell suggested.

@aduh95 aduh95 force-pushed the add-support-for-url-in-worker-constructor branch from b65b8a0 to dde6e19 Compare February 6, 2020 20:05
addaleax
addaleax reviewed Feb 6, 2020
View reviewed changes
doc/api/errors.md Outdated Show resolved Hide resolved
lib/internal/worker.js Outdated Show resolved Hide resolved
richardlau
richardlau reviewed Feb 6, 2020
View reviewed changes
lib/internal/worker.js Outdated Show resolved Hide resolved
@aduh95 aduh95 force-pushed the add-support-for-url-in-worker-constructor branch from dde6e19 to 5d1ea0b Compare February 11, 2020 17:33
@aduh95
Copy link
Contributor Author

aduh95 commented Feb 11, 2020

I have made some changes to address previous comments.

I am not using toPathIfFileURL not getValidatedPath, my reasoning is that when using the new Worker(new URL) pattern, the URL would be converted back and forth (file:// string -> URL -> path string -> URL); I haven't run any benchmark to test if that matters or not, if you think this optimisation is not necessary, let me know.

I have also changed the ERR_WORKER_PATH error message to give a hint to users trying to use new Worker(import.meta.url).

addaleax
addaleax approved these changes Feb 13, 2020
View reviewed changes
lib/internal/errors.js Outdated Show resolved Hide resolved
@addaleax addaleax requested a review from jasnell February 13, 2020 16:12
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/29099/

@addaleax
Copy link
Member

Ping @aduh95 – this would need to be rebased, and there are a few comments that should be addressed here.

Also, can you add Fixes: https://github.com/nodejs/node/issues/30780 to the commit message? (This should fix #30780.)

@aduh95 aduh95 force-pushed the add-support-for-url-in-worker-constructor branch from 5d1ea0b to 5f2d23a Compare February 20, 2020 13:46
@aduh95 aduh95 requested a review from addaleax February 20, 2020 14:13
@aduh95 aduh95 force-pushed the add-support-for-url-in-worker-constructor branch from 5f2d23a to d201af2 Compare February 20, 2020 14:16
addaleax
addaleax approved these changes Feb 20, 2020
View reviewed changes
lib/internal/errors.js Outdated Show resolved Hide resolved
lib/internal/worker.js Outdated Show resolved Hide resolved
@aduh95 aduh95 force-pushed the add-support-for-url-in-worker-constructor branch 2 times, most recently from f1afafd to 33324ec Compare February 20, 2020 16:21
@guybedford guybedford mentioned this pull request Feb 21, 2020
Open
@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 22, 2020
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/29283/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/29644/

addaleax pushed a commit that referenced this pull request Mar 13, 2020
@aduh95 @addaleax
worker: allow URL in Worker constructor
a8b3d7b 
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 <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@aduh95 aduh95 deleted the add-support-for-url-in-worker-constructor branch March 13, 2020 09:55
@cjihrig cjihrig mentioned this pull request Mar 15, 2020
Closed
3 tasks
BridgeAR pushed a commit that referenced this pull request Mar 17, 2020
@aduh95 @BridgeAR
worker: allow URL in Worker constructor
435fbbc 
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 <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
jasnell pushed a commit that referenced this pull request Mar 17, 2020
@cjihrig @jasnell
errors: drop pronouns from ERR_WORKER_PATH message
c788964 
This commit drops pronouns from the ERR_WORKER_PATH message,
and also shortens the text a bit.

PR-URL: #32285
Refs: #31664
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 19, 2020
@cjihrig @MylesBorins
errors: drop pronouns from ERR_WORKER_PATH message
e60efa7 
This commit drops pronouns from the ERR_WORKER_PATH message,
and also shortens the text a bit.

PR-URL: #32285
Refs: #31664
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
MylesBorins added a commit that referenced this pull request Mar 19, 2020
@MylesBorins
2020-03-24 Version 13.12.0 (Current)
8fc5a91 
Notable changes:

* [a44da56] - (SEMVER-MINOR) doc: update stability of report features (Colin Ihrig) #32242
* [306ed96] - (SEMVER-MINOR) doc,lib,src,test: make --experimental-report a nop (Colin Ihrig) #32242
* [ea7f89d] - (SEMVER-MINOR) test: remove common.skipIfReportDisabled() (Colin Ihrig) #32242
* [3f1f518] - (SEMVER-MINOR) build: make --without-report a no-op (Colin Ihrig) #32242
* [36ab39f] - (SEMVER-MINOR) build: remove node_report option in node.gyp (Colin Ihrig) #32242
* [514b7c2] - (SEMVER-MINOR) src: unconditionally include report feature (Colin Ihrig) #32242
* [435fbbc] - (SEMVER-MINOR) worker: allow URL in Worker constructor (Antoine du HAMEL) #31664
* [975d6b0] - (SEMVER-MINOR) util: use a global symbol for `util.promisify.custom` (ExE Boss) #31672
@MylesBorins MylesBorins mentioned this pull request Mar 19, 2020
Merged
MylesBorins pushed a commit that referenced this pull request Mar 24, 2020
@aduh95 @MylesBorins
worker: allow URL in Worker constructor
e83dcde 
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 <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 24, 2020
@cjihrig @MylesBorins
errors: drop pronouns from ERR_WORKER_PATH message
71a2fa2 
This commit drops pronouns from the ERR_WORKER_PATH message,
and also shortens the text a bit.

PR-URL: #32285
Refs: #31664
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
MylesBorins added a commit that referenced this pull request Mar 24, 2020
@MylesBorins
2020-03-25 Version 13.12.0 (Current)
968751d 
Notable changes:

* build:
  * macOS package notarization (Rod Vagg)
    #31459
* deps:
  * update npm to 6.14.3 (Myles Borins)
    #32368
  * update to uvwasi 0.0.6 (Colin Ihrig) [#32309](#32309)
  * upgrade to libuv 1.35.0 (Colin Ihrig)
    #32204
* node\_report:
  * move diagnostic reports to stable (Colin Ihrig)
    #32242
* worker:
  * allow URL in Worker constructor (Antoine du HAMEL)
    #31664
* util:
  * use a global symbol for `util.promisify.custom` (ExE Boss)
    #31672

PR-URL: #32376
MylesBorins added a commit that referenced this pull request Mar 24, 2020
@MylesBorins
2020-03-25 Version 13.12.0 (Current)
27dbe1f 
Notable changes:

* build:
  * macOS package notarization (Rod Vagg)
    #31459
* deps:
  * update npm to 6.14.3 (Myles Borins)
    #32368
  * update to uvwasi 0.0.6 (Colin Ihrig) [#32309](#32309)
  * upgrade to libuv 1.35.0 (Colin Ihrig)
    #32204
* node\_report:
  * move diagnostic reports to stable (Colin Ihrig)
    #32242
* worker:
  * allow URL in Worker constructor (Antoine du HAMEL)
    #31664
* util:
  * use a global symbol for `util.promisify.custom` (ExE Boss)
    #31672

PR-URL: #32376
MylesBorins added a commit that referenced this pull request Mar 25, 2020
@MylesBorins
2020-03-26 Version 13.12.0 (Current)
a87ac59 
macOS package notarization and a change in builder configuration:

The macOS binaries for this release, and future 13.x releases, are now
being compiled on macOS 10.15 (Catalina) with Xcode 11 to support
package notarization, a requirement for installing on .pkg files on
macOS 10.15 and later. Previous builds of Node.js 13.x were compiled on
macOS 10.11 (El Capitan) with Xcode 10. As binaries are still being
compiled to support a minimum of macOS 10.10 (Yosemite) we do not
anticipate this having a negative impact on Node.js 13.x users with
older versions of macOS.

Notable changes:

* build:
  * macOS package notarization (Rod Vagg)
    #31459
* deps:
  * update npm to 6.14.3 (Myles Borins)
    #32368
  * update to uvwasi 0.0.6 (Colin Ihrig)
    #32309
  * upgrade to libuv 1.35.0 (Colin Ihrig)
    #32204
* lib:
  * add --disable-proto option to cli (Gus Caplan)
    #32279
* node_report:
  * move diagnostic reports to stable (Colin Ihrig)
    #32242
* worker:
  * allow URL in Worker constructor (Antoine du HAMEL)
    #31664
* util:
  * use a global symbol for `util.promisify.custom` (ExE Boss)
    #31672

PR-URL: #32376
MylesBorins added a commit that referenced this pull request Mar 26, 2020
@MylesBorins
2020-03-26 Version 13.12.0 (Current)
65782ff 
macOS package notarization and a change in builder configuration:

The macOS binaries for this release, and future 13.x releases, are now
being compiled on macOS 10.15 (Catalina) with Xcode 11 to support
package notarization, a requirement for installing on .pkg files on
macOS 10.15 and later. Previous builds of Node.js 13.x were compiled on
macOS 10.11 (El Capitan) with Xcode 10. As binaries are still being
compiled to support a minimum of macOS 10.10 (Yosemite) we do not
anticipate this having a negative impact on Node.js 13.x users with
older versions of macOS.

Notable changes:

* build:
  * macOS package notarization (Rod Vagg)
    #31459
* deps:
  * upgrade npm to 6.14.4 (Ruy Adorno)
    #32495
  * update to uvwasi 0.0.6 (Colin Ihrig)
    #32309
  * upgrade to libuv 1.35.0 (Colin Ihrig)
    #32204
* lib:
  * add --disable-proto option to cli (Gus Caplan)
    #32279
* node_report:
  * move diagnostic reports to stable (Colin Ihrig)
    #32242
* worker:
  * allow URL in Worker constructor (Antoine du HAMEL)
    #31664
* util:
  * use a global symbol for `util.promisify.custom` (ExE Boss)
    #31672

PR-URL: #32376
MylesBorins added a commit that referenced this pull request Mar 26, 2020
@MylesBorins
2020-03-26 Version 13.12.0 (Current)
f6a339f 
macOS package notarization and a change in builder configuration:

The macOS binaries for this release, and future 13.x releases, are now
being compiled on macOS 10.15 (Catalina) with Xcode 11 to support
package notarization, a requirement for installing on .pkg files on
macOS 10.15 and later. Previous builds of Node.js 13.x were compiled on
macOS 10.11 (El Capitan) with Xcode 10. As binaries are still being
compiled to support a minimum of macOS 10.10 (Yosemite) we do not
anticipate this having a negative impact on Node.js 13.x users with
older versions of macOS.

Notable changes:

* build:
  * macOS package notarization (Rod Vagg)
    #31459
* deps:
  * upgrade npm to 6.14.4 (Ruy Adorno)
    #32495
  * update to uvwasi 0.0.6 (Colin Ihrig)
    #32309
  * upgrade to libuv 1.35.0 (Colin Ihrig)
    #32204
* lib:
  * add --disable-proto option to cli (Gus Caplan)
    #32279
* node_report:
  * move diagnostic reports to stable (Colin Ihrig)
    #32242
* worker:
  * allow URL in Worker constructor (Antoine du HAMEL)
    #31664
* util:
  * use a global symbol for `util.promisify.custom` (ExE Boss)
    #31672

PR-URL: #32376
MylesBorins added a commit that referenced this pull request Mar 26, 2020
@MylesBorins
2020-03-26 Version 13.12.0 (Current)
113c04c 
macOS package notarization and a change in builder configuration:

The macOS binaries for this release, and future 13.x releases, are now
being compiled on macOS 10.15 (Catalina) with Xcode 11 to support
package notarization, a requirement for installing on .pkg files on
macOS 10.15 and later. Previous builds of Node.js 13.x were compiled on
macOS 10.11 (El Capitan) with Xcode 10. As binaries are still being
compiled to support a minimum of macOS 10.10 (Yosemite) we do not
anticipate this having a negative impact on Node.js 13.x users with
older versions of macOS.

Notable changes:

* build:
  * macOS package notarization (Rod Vagg)
    #31459
* deps:
  * upgrade npm to 6.14.4 (Ruy Adorno)
    #32495
  * update to uvwasi 0.0.6 (Colin Ihrig)
    #32309
  * upgrade to libuv 1.35.0 (Colin Ihrig)
    #32204
* lib:
  * add --disable-proto option to cli (Gus Caplan)
    #32279
* node_report:
  * move diagnostic reports to stable (Colin Ihrig)
    #32242
* worker:
  * allow URL in Worker constructor (Antoine du HAMEL)
    #31664
* util:
  * use a global symbol for `util.promisify.custom` (ExE Boss)
    #31672

PR-URL: #32376
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
@aduh95 @targos
worker: allow URL in Worker constructor
c5c9e54 
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 <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
@cjihrig @targos
errors: drop pronouns from ERR_WORKER_PATH message
9225c64 
This commit drops pronouns from the ERR_WORKER_PATH message,
and also shortens the text a bit.

PR-URL: nodejs#32285
Refs: nodejs#31664
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@targos targos removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 25, 2020
targos pushed a commit that referenced this pull request Apr 28, 2020
@aduh95 @targos
worker: allow URL in Worker constructor
217e3df 
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 <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
targos pushed a commit that referenced this pull request Apr 28, 2020
@cjihrig @targos
errors: drop pronouns from ERR_WORKER_PATH message
ea4302b 
This commit drops pronouns from the ERR_WORKER_PATH message,
and also shortens the text a bit.

PR-URL: #32285
Refs: #31664
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@targos targos mentioned this pull request May 2, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardlau richardlau richardlau left review comments

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

@devsnek devsnek devsnek approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

Assignees
No one assigned
Labels
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@aduh95 @addaleax @jasnell @nodejs-github-bot @joyeecheung @richardlau @devsnek @targos