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

process: make Symbol.toStringTag writable #26488

Closed
wants to merge 1 commit into from
Closed

process: make Symbol.toStringTag writable #26488

wants to merge 1 commit into from

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Mar 7, 2019

The ecosystem broke by making it non-writable, so this is a good
intermediate fix.

Refs: #25963 (comment)

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

@BridgeAR
process: make Symbol.toStringTag writable
39664bc 
The ecosystem broke by making it non-writable, so this is a good
intermediate fix.
@nodejs-github-bot
Copy link
Collaborator

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

@BridgeAR
Copy link
Member Author

BridgeAR commented Mar 7, 2019

We could switch this to use getter/setters so we are able to deprecate setting this property as semver-major.

@SimenB SimenB mentioned this pull request Mar 7, 2019
Closed
3 tasks
@guybedford
Copy link
Contributor

Actually making it a getter so we can deprecate it as major does sound sensible. Especially given Jest already have a fix here. Shall we do a docs-only deprecation here then?

@SimenB SimenB mentioned this pull request Mar 7, 2019
Closed
@xxx xxx mentioned this pull request Mar 7, 2019
Merged
@hiroppy hiroppy mentioned this pull request Mar 8, 2019
Merged
6 tasks
@BridgeAR
Copy link
Member Author

BridgeAR commented Mar 8, 2019 •
edited
Loading

CI https://ci.nodejs.org/job/node-test-pull-request/21333/
CI https://ci.nodejs.org/job/node-test-pull-request/21377/ ✔️

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 8, 2019
@richardlau richardlau mentioned this pull request Mar 8, 2019
Closed
@micalgenus micalgenus mentioned this pull request Mar 9, 2019
Closed
@Hacklor Hacklor mentioned this pull request Mar 11, 2019
Merged
BridgeAR added a commit to BridgeAR/node that referenced this pull request Mar 12, 2019
@BridgeAR
process: make Symbol.toStringTag writable
2a7da11 
The ecosystem broke by making it non-writable, so this is a good
intermediate fix.

PR-URL: nodejs#26488
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Yuta Hiroto <hello@hiroppy.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR
Copy link
Member Author

Landed in 2a7da11 🎉

@BridgeAR BridgeAR closed this Mar 12, 2019
BridgeAR added a commit that referenced this pull request Mar 13, 2019
@BridgeAR
process: make Symbol.toStringTag writable
c6536b4 
The ecosystem broke by making it non-writable, so this is a good
intermediate fix.

PR-URL: #26488
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Yuta Hiroto <hello@hiroppy.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Mar 13, 2019
Merged
BridgeAR added a commit that referenced this pull request Mar 13, 2019
@BridgeAR
2019-03-14, Version 11.12.0 (Current)
589c1b6 
Notable Changes

* build:
  * Enable v8's siphash for hash seed creation (Rod Vagg)
    #26367
* crypto:
  * Add `KeyObject.asymmetricKeySize` (Patrick Gansterer)
    #26387
* deps:
  * Upgrade openssl to 1.1.1b (Sam Roberts)
    #26327
* process:
  * Make `process[Symbol.toStringTag]` writable again
    (Ruben Bridgewater) #26488
* repl:
  * Add `util.inspect.replDefaults` to customize the writer
    (Ruben Bridgewater) #26375
* report:
  * Rename `triggerReport()` to `writeReport()` (Colin Ihrig)
    #26527
@BridgeAR
Copy link
Member Author

@guybedford are you going to open a PR for the deprecation? I personally have no strong opinion about that.

BridgeAR added a commit that referenced this pull request Mar 14, 2019
@BridgeAR
process: make Symbol.toStringTag writable
727ade1 
The ecosystem broke by making it non-writable, so this is a good
intermediate fix.

PR-URL: #26488
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Yuta Hiroto <hello@hiroppy.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR added a commit that referenced this pull request Mar 14, 2019
@BridgeAR
2019-03-14, Version 11.12.0 (Current)
cb1e0cb 
Notable Changes

* bootstrap:
  * Add experimental `--frozen-intrinsics` flag (Guy Bedford)
    #25685
* build:
  * Enable v8's siphash for hash seed creation (Rod Vagg)
    #26367
* deps:
  * Upgrade openssl to 1.1.1b (Sam Roberts)
    #26327
* process:
  * Make `process[Symbol.toStringTag]` writable again
    (Ruben Bridgewater) #26488
* repl:
  * Add `util.inspect.replDefaults` to customize the writer
    (Ruben Bridgewater) #26375
* report:
  * Rename `triggerReport()` to `writeReport()` (Colin Ihrig)
    #26527
@guybedford
Copy link
Contributor

@BridgeAR I know it seems small, but intrinsics have non-writeable toStringTags because they are supposed to provide a certain level of consistency so I do think it would be more correct to do this. I don't feel particularly strongly about it either, but will aim to get to the PR as it seems better than not.

BridgeAR added a commit to BridgeAR/node that referenced this pull request Mar 14, 2019
@BridgeAR
process: make Symbol.toStringTag writable
f48c697 
The ecosystem broke by making it non-writable, so this is a good
intermediate fix.

PR-URL: nodejs#26488
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Yuta Hiroto <hello@hiroppy.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Mar 14, 2019
@BridgeAR
2019-03-14, Version 11.12.0 (Current)
f83918b 
Notable Changes

* bootstrap:
  * Add experimental `--frozen-intrinsics` flag (Guy Bedford)
    nodejs#25685
* build:
  * Enable v8's siphash for hash seed creation (Rod Vagg)
    nodejs#26367
* deps:
  * Upgrade openssl to 1.1.1b (Sam Roberts)
    nodejs#26327
* process:
  * Make `process[Symbol.toStringTag]` writable again
    (Ruben Bridgewater) nodejs#26488
* repl:
  * Add `util.inspect.replDefaults` to customize the writer
    (Ruben Bridgewater) nodejs#26375
* report:
  * Rename `triggerReport()` to `writeReport()` (Colin Ihrig)
    nodejs#26527
BridgeAR added a commit that referenced this pull request Mar 14, 2019
@BridgeAR
process: make Symbol.toStringTag writable
275a8f9 
The ecosystem broke by making it non-writable, so this is a good
intermediate fix.

PR-URL: #26488
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Yuta Hiroto <hello@hiroppy.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR added a commit that referenced this pull request Mar 14, 2019
@BridgeAR
2019-03-14, Version 11.12.0 (Current)
274f8ae 
Notable Changes

* bootstrap:
  * Add experimental `--frozen-intrinsics` flag (Guy Bedford)
    #25685
* build:
  * Enable v8's siphash for hash seed creation (Rod Vagg)
    #26367
* deps:
  * Upgrade openssl to 1.1.1b (Sam Roberts)
    #26327
* process:
  * Make `process[Symbol.toStringTag]` writable again
    (Ruben Bridgewater) #26488
* repl:
  * Add `util.inspect.replDefaults` to customize the writer
    (Ruben Bridgewater) #26375
* report:
  * Rename `triggerReport()` to `writeReport()` (Colin Ihrig)
    #26527
BridgeAR added a commit that referenced this pull request Mar 15, 2019
@BridgeAR
2019-03-15, Version 11.12.0 (Current)
a4ff9df 
Notable Changes

* bootstrap:
  * Add experimental `--frozen-intrinsics` flag (Guy Bedford)
    #25685
* build:
  * Enable v8's siphash for hash seed creation (Rod Vagg)
    #26367
* deps:
  * Upgrade openssl to 1.1.1b (Sam Roberts)
    #26327
* process:
  * Make `process[Symbol.toStringTag]` writable again
    (Ruben Bridgewater) #26488
* repl:
  * Add `util.inspect.replDefaults` to customize the writer
    (Ruben Bridgewater) #26375
* report:
  * Rename `triggerReport()` to `writeReport()` (Colin Ihrig)
    #26527
BridgeAR added a commit to BridgeAR/node that referenced this pull request Mar 15, 2019
@BridgeAR
2019-03-15, Version 11.12.0 (Current)
969c63a 
Notable Changes

* bootstrap:
  * Add experimental `--frozen-intrinsics` flag (Guy Bedford)
    nodejs#25685
* build:
  * Enable v8's siphash for hash seed creation (Rod Vagg)
    nodejs#26367
* deps:
  * Upgrade openssl to 1.1.1b (Sam Roberts)
    nodejs#26327
* process:
  * Make `process[Symbol.toStringTag]` writable again
    (Ruben Bridgewater) nodejs#26488
* repl:
  * Add `util.inspect.replDefaults` to customize the writer
    (Ruben Bridgewater) nodejs#26375
* report:
  * Rename `triggerReport()` to `writeReport()` (Colin Ihrig)
    nodejs#26527
@guybedford guybedford mentioned this pull request Mar 17, 2019
Closed
4 tasks
@julienw julienw mentioned this pull request Mar 22, 2019
Closed
@liangchunn liangchunn mentioned this pull request Mar 25, 2019
Closed
@BridgeAR BridgeAR deleted the fix-jest-process-tag branch January 20, 2020 11:55
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

@targos targos targos approved these changes

@hiroppy hiroppy hiroppy approved these changes

@himself65 himself65 himself65 approved these changes

@addaleax addaleax Awaiting requested review from addaleax

@guybedford guybedford Awaiting requested review from guybedford

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@BridgeAR @nodejs-github-bot @guybedford @jasnell @targos @hiroppy @himself65