GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — worker: remove `--experimental-worker` flag by addaleax · Pull Request #25361 · nodejs/node

Conversation

@addaleax

@addaleax addaleax commented Jan 6, 2019

Copy link
Copy Markdown
Member

Having an experimental feature behind a flag makes change
if we are expecting significant breaking changes to its API.

Since the Worker API has been essentially stable since
its initial introduction, and no noticeable doubt about
possibly not keeping the feature around has been voiced,
removing the flag and thereby reducing the barrier to experimentation,
and consequently receiving feedback on the implementation,
seems like a good idea.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

hiroppy, sindresorhus, bricss, ChALkeR, benjamingr, shisama, SerayaEryn, TimothyGu, larshp, rotem-bar, and 8 more reacted with thumbs up emoji benjamingr, eugeneo, yaelhe, bricss, mourner, EKashpersky, temarch, primary-1, mwanago, lararosekelley, and 2 more reacted with hooray emoji benjamingr, bricss, rotem-bar, primary-1, mwanago, and saeedtabrizi reacted with heart emoji
All reactions
  • 👍 18 reactions
  • 🎉 12 reactions
  • ❤️ 6 reactions
@addaleax
worker: remove --experimental-worker flag
7d20ade
Having an experimental feature behind a flag makes change
if we are expecting significant breaking changes to its API.

Since the Worker API has been essentially stable since
its initial introduction, and no noticeable doubt about
possibly not keeping the feature around has been voiced,
removing the flag and thereby reducing the barrier to experimentation,
and consequently receiving feedback on the implementation,
seems like a good idea.
@addaleax addaleax added the worker Issues and PRs related to the worker_threads module and Worker API. label Jan 6, 2019
@nodejs-github-bot

nodejs-github-bot commented Jan 6, 2019

Copy link
Copy Markdown
Collaborator

@addaleax build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2149/pipeline

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. labels Jan 6, 2019
@addaleax

addaleax commented Jan 6, 2019

Copy link
Copy Markdown
Member Author

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

/cc @nodejs/workers

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Trott
Trott reviewed Jan 6, 2019
View reviewed changes
Comment thread benchmark/misc/startup.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

Trott
Trott approved these changes Jan 6, 2019
View reviewed changes
hiroppy
hiroppy approved these changes Jan 6, 2019
View reviewed changes

@hiroppy hiroppy left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😆

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Leko
Leko approved these changes Jan 6, 2019
View reviewed changes
jasnell
jasnell approved these changes Jan 6, 2019
View reviewed changes
cjihrig
cjihrig approved these changes Jan 6, 2019
View reviewed changes
benjamingr
benjamingr approved these changes Jan 6, 2019
View reviewed changes
@addaleax
fixup! worker: remove --experimental-worker flag
e92f055
@addaleax

addaleax commented Jan 6, 2019

Copy link
Copy Markdown
Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@addaleax addaleax added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Jan 6, 2019
TimothyGu
TimothyGu approved these changes Jan 6, 2019
View reviewed changes
tniessen
tniessen approved these changes Jan 6, 2019
View reviewed changes
shisama
shisama approved these changes Jan 7, 2019
View reviewed changes
@gireeshpunathil

gireeshpunathil commented Jan 7, 2019

Copy link
Copy Markdown
Member

@addaleax - I was under the impression that the module name worker_threads is a place holder that can converge and freeze into something else before exiting experimental. Isn't that so?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

starkwang
starkwang approved these changes Jan 7, 2019
View reviewed changes
@addaleax

addaleax commented Jan 8, 2019

Copy link
Copy Markdown
Member Author

@gireeshpunathil If you want to pick another name, we can take up that conversation again – I think it’s not too late if we really want to changes this, but I’m not sure it’s worth it.

worker_threads is accurate and not too explicit as a name, imo. It wasn’t my first choice, but I’d rather leave it as it is than start the whole discussion again (and break the existing code that is out there, including guides/tutorials, even if it’s currently still a flagged feature).

benjamingr reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@gireeshpunathil

gireeshpunathil commented Jan 8, 2019

Copy link
Copy Markdown
Member

@addaleax - thanks, I agree. I was just making sure my understanding around the naming is correct, and wanted to confirm it as this is the last opportunity to talk about that, if at all. Agree that worker_threads is an appropriate name within the constraints (being meaningful and not overlapping with used names).

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

gireeshpunathil
gireeshpunathil approved these changes Jan 8, 2019
View reviewed changes
mhdawson
mhdawson approved these changes Jan 8, 2019
View reviewed changes

@mhdawson mhdawson left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, assuming the feature is still documented as experimental.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@addaleax

addaleax commented Jan 8, 2019

Copy link
Copy Markdown
Member Author

Landed in 63d4cae

Thanks for the reviews, everyone!

tniessen and eljefedelrodeodeljefe reacted with hooray emoji
All reactions
  • 🎉 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@addaleax addaleax closed this Jan 8, 2019
@addaleax
addaleax deleted the worker-no-flag branch January 8, 2019 23:44
Trott pushed a commit to Trott/io.js that referenced this pull request Jan 8, 2019
@addaleax
worker: remove --experimental-worker flag
63d4cae
Having an experimental feature behind a flag makes change
if we are expecting significant breaking changes to its API.

Since the Worker API has been essentially stable since
its initial introduction, and no noticeable doubt about
possibly not keeping the feature around has been voiced,
removing the flag and thereby reducing the barrier to experimentation,
and consequently receiving feedback on the implementation,
seems like a good idea.

PR-URL: nodejs#25361
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Shingo Inoue <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Masashi Hirano <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@addaleax addaleax added the backport-requested-v11.x label Jan 9, 2019
addaleax added a commit to addaleax/node that referenced this pull request Jan 9, 2019
@addaleax
worker: set --experimental-worker always
18118b2
This is similar to nodejs#25361 in
functionality, but allows avoiding some backporting pain for v11.x.

Refs: nodejs#25361
addaleax added a commit to addaleax/node that referenced this pull request Jan 9, 2019
@addaleax
worker: partially remove --experimental-worker flag
29a697f
This is a trimmed-down version of 63d4cae that avoids
backporting pain for v11.x. The remainder of the original commit
can be cherry-picked later, once other PRs have been backported first.

---

Having an experimental feature behind a flag makes change
if we are expecting significant breaking changes to its API.

Since the Worker API has been essentially stable since
its initial introduction, and no noticeable doubt about
possibly not keeping the feature around has been voiced,
removing the flag and thereby reducing the barrier to experimentation,
and consequently receiving feedback on the implementation,
seems like a good idea.

Refs: nodejs#25361
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Shingo Inoue <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Masashi Hirano <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@addaleax addaleax mentioned this pull request Jan 9, 2019
[v11.x] enable workers by default #25404
Closed
4 tasks done
@BridgeAR

BridgeAR commented Jan 9, 2019

Copy link
Copy Markdown
Member

This is functionally backported to v11.x but some parts are not yet backported which await other commits to be backported first.

addaleax reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

BridgeAR pushed a commit that referenced this pull request Jan 10, 2019
@addaleax @BridgeAR
worker: set --experimental-worker always
7bb7b9a
This is similar to #25361 in
functionality, but allows avoiding some backporting pain for v11.x.

PR-URL: #25404
Refs: #25361
Reviewed-By: Ruben Bridgewater <[email protected]>
BridgeAR pushed a commit that referenced this pull request Jan 10, 2019
@addaleax @BridgeAR
worker: partially remove --experimental-worker flag
65c136f
This is a trimmed-down version of 63d4cae that avoids
backporting pain for v11.x. The remainder of the original commit
can be cherry-picked later, once other PRs have been backported first.

PR-URL: #25404
Reviewed-By: Ruben Bridgewater <[email protected]>

--- Original commit message ---

Having an experimental feature behind a flag makes change
if we are expecting significant breaking changes to its API.

Since the Worker API has been essentially stable since
its initial introduction, and no noticeable doubt about
possibly not keeping the feature around has been voiced,
removing the flag and thereby reducing the barrier to experimentation,
and consequently receiving feedback on the implementation,
seems like a good idea.

Refs: #25361
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Shingo Inoue <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Masashi Hirano <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@addaleax addaleax removed the backport-requested-v11.x label Jan 15, 2019
addaleax added a commit that referenced this pull request Jan 15, 2019
@addaleax
worker: remove --experimental-worker flag
ab7284a
Having an experimental feature behind a flag makes change
if we are expecting significant breaking changes to its API.

Since the Worker API has been essentially stable since
its initial introduction, and no noticeable doubt about
possibly not keeping the feature around has been voiced,
removing the flag and thereby reducing the barrier to experimentation,
and consequently receiving feedback on the implementation,
seems like a good idea.

PR-URL: #25361
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Shingo Inoue <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Masashi Hirano <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Jan 16, 2019
v11.7.0 proposal #25537
Merged
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
@addaleax @BridgeAR
worker: set --experimental-worker always
641cd94
This is similar to nodejs#25361 in
functionality, but allows avoiding some backporting pain for v11.x.

PR-URL: nodejs#25404
Refs: nodejs#25361
Reviewed-By: Ruben Bridgewater <[email protected]>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
@addaleax @BridgeAR
worker: partially remove --experimental-worker flag
fe10881
This is a trimmed-down version of 63d4cae that avoids
backporting pain for v11.x. The remainder of the original commit
can be cherry-picked later, once other PRs have been backported first.

PR-URL: nodejs#25404
Reviewed-By: Ruben Bridgewater <[email protected]>

--- Original commit message ---

Having an experimental feature behind a flag makes change
if we are expecting significant breaking changes to its API.

Since the Worker API has been essentially stable since
its initial introduction, and no noticeable doubt about
possibly not keeping the feature around has been voiced,
removing the flag and thereby reducing the barrier to experimentation,
and consequently receiving feedback on the implementation,
seems like a good idea.

Refs: nodejs#25361
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Shingo Inoue <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Masashi Hirano <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
@addaleax @BridgeAR
worker: remove --experimental-worker flag
36b1e7a
Having an experimental feature behind a flag makes change
if we are expecting significant breaking changes to its API.

Since the Worker API has been essentially stable since
its initial introduction, and no noticeable doubt about
possibly not keeping the feature around has been voiced,
removing the flag and thereby reducing the barrier to experimentation,
and consequently receiving feedback on the implementation,
seems like a good idea.

PR-URL: nodejs#25361
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Shingo Inoue <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Masashi Hirano <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 17, 2019
@addaleax @BridgeAR
worker: remove --experimental-worker flag
243f902
Having an experimental feature behind a flag makes change
if we are expecting significant breaking changes to its API.

Since the Worker API has been essentially stable since
its initial introduction, and no noticeable doubt about
possibly not keeping the feature around has been voiced,
removing the flag and thereby reducing the barrier to experimentation,
and consequently receiving feedback on the implementation,
seems like a good idea.

PR-URL: nodejs#25361
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Shingo Inoue <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Masashi Hirano <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@addaleax addaleax added the notable-change PRs with changes that should be highlighted in changelogs. label Jan 17, 2019
BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 17, 2019
@BridgeAR
2019-01-17, Version 11.7.0 (Current), @BridgeAR
6b7b827
Notable Changes

* compression / zlib:
  * Added brotli support (Anna Henningsen and Zach Vacura)
    nodejs#24938
* console:
  * Added `inspectOptions` option (Ruben Bridgewater)
    nodejs#24978
* crypto:
  * Always accept private keys as public keys (Tobias Nießen)
    nodejs#25217
* deps:
  * Upgrade npm to v6.5.0 (Jordan Harband)
    nodejs#25234
* fs:
  * Use internalBinding('fs') internally instead of
    process.binding('fs') (Masashi Hirano)
    nodejs#22478
* http(s):
  * Support overriding http\\s.globalAgent (Roy Sommer)
    nodejs#25170
* util:
  * Inspect ArrayBuffers contents closely (Ruben Bridgewater)
    nodejs#25006
* worker:
  * Expose workers by default and remove `--experimental-worker` flag
    (Anna Henningsen) nodejs#25361

PR-URL: nodejs#25537
BridgeAR added a commit that referenced this pull request Jan 18, 2019
@BridgeAR
2019-01-17, Version 11.7.0 (Current), @BridgeAR
d3f8f90
Notable Changes

* compression / zlib:
  * Added brotli support (Anna Henningsen and Zach Vacura)
    #24938
* console:
  * Added `inspectOptions` option (Ruben Bridgewater)
    #24978
* crypto:
  * Always accept private keys as public keys (Tobias Nießen)
    #25217
* deps:
  * Upgrade npm to v6.5.0 (Jordan Harband)
    #25234
* fs:
  * Use internalBinding('fs') internally instead of
    process.binding('fs') (Masashi Hirano)
    #22478
* http(s):
  * Support overriding http\\s.globalAgent (Roy Sommer)
    #25170
* util:
  * Inspect ArrayBuffers contents closely (Ruben Bridgewater)
    #25006
* worker:
  * Expose workers by default and remove `--experimental-worker` flag
    (Anna Henningsen) #25361

PR-URL: #25537
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2019
v11.8.0 proposal #25687
Merged
sebdeckers added a commit to commonshost/bulldohzer that referenced this pull request Feb 4, 2019
@sebdeckers
fix: 11.7.0 is where workers were no longer behind experimental flag
b63ef0a
nodejs/node#25361
@Suvitruf Suvitruf mentioned this pull request Mar 8, 2019
bcrypt crashes in child process kelektiv/node.bcrypt.js#709
Closed
@richardlau richardlau mentioned this pull request Jan 30, 2020
src: remove --experimental-worker env option #31577
Closed
1 of 4 tasks
@skyclouds2001 skyclouds2001 mentioned this pull request Nov 9, 2024
Node.js 11.7+ supports Worker + related updates mdn/browser-compat-data#25012
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes
@Trott Trott Trott approved these changes
@benjamingr benjamingr benjamingr approved these changes
@cjihrig cjihrig cjihrig approved these changes
@tniessen tniessen tniessen approved these changes
@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes
+6 more reviewers
@Leko Leko Leko approved these changes
@TimothyGu TimothyGu TimothyGu approved these changes
@hiroppy hiroppy hiroppy approved these changes
@mhdawson mhdawson mhdawson approved these changes
@shisama shisama shisama approved these changes
@starkwang starkwang starkwang approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. notable-change PRs with changes that should be highlighted in changelogs. worker Issues and PRs related to the worker_threads module and Worker API.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

15 participants

@addaleax @nodejs-github-bot @gireeshpunathil @BridgeAR @jasnell @Trott @benjamingr @Leko @TimothyGu @cjihrig @tniessen @hiroppy @mhdawson @shisama @starkwang
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.