tty: add NO_COLOR and FORCE_COLOR support by BridgeAR · Pull Request #26485 · nodejs/node · GitHub
Skip to content

tty: add NO_COLOR and FORCE_COLOR support#26485

Closed
BridgeAR wants to merge 2 commits intonodejs:masterfrom
BridgeAR:add-force-color
Closed

tty: add NO_COLOR and FORCE_COLOR support#26485
BridgeAR wants to merge 2 commits intonodejs:masterfrom
BridgeAR:add-force-color

Conversation

@BridgeAR
Copy link
Copy Markdown
Member

@BridgeAR BridgeAR commented Mar 7, 2019

This adds support to enforce a specific color depth by checking the
FORCE_COLOR environment variable similar to chalk.

On top of that we also add support for the NO_COLOR environment
variable as suggested by https://no-color.org/.

Refs: #26248

This supersedes #26248.

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
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented Mar 7, 2019

@nodejs-github-bot nodejs-github-bot added the tty Issues and PRs related to the tty subsystem. label Mar 7, 2019
@BridgeAR BridgeAR mentioned this pull request Mar 7, 2019
Closed
4 tasks
@BridgeAR BridgeAR requested a review from addaleax March 7, 2019 01:15
@BridgeAR
Copy link
Copy Markdown
Member Author

BridgeAR commented Mar 7, 2019 •
edited
Loading

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

@BridgeAR BridgeAR added the semver-minor PRs that contain new features and should be released in the next minor version. label Mar 7, 2019
@BridgeAR
tty: add NO_COLOR and FORCE_COLOR support
af0b138 
This adds support to enforce a specific color depth by checking the
`FORCE_COLOR` environment variable similar to `chalk`.

On top of that we also add support for the `NO_COLOR` environment
variable as suggested by https://no-color.org/.
@BridgeAR BridgeAR mentioned this pull request Mar 7, 2019
Closed
silverwind
silverwind approved these changes Mar 8, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@silverwind silverwind left a comment

Choose a reason for hiding this comment

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

LGTM, thought some sort of standardization attempt for FORCE_COLOR and especially its values would be nice.

@BridgeAR
Copy link
Copy Markdown
Member Author

BridgeAR commented Mar 8, 2019

@silverwind I am already working in that direction and opened issues in the no-color repository and want to achieve some consolidations with true color support and things like that and suggest some things to some people who work on terminal colors.

@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
vsemozhetbyt
vsemozhetbyt reviewed Mar 8, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@vsemozhetbyt vsemozhetbyt left a comment

Choose a reason for hiding this comment

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

Doc format LGTM with nits.

Comment thread doc/api/tty.md Outdated
Comment thread doc/api/tty.md Outdated
Comment thread doc/api/tty.md Outdated
Comment thread doc/api/tty.md Outdated
@BridgeAR
Copy link
Copy Markdown
Member Author

BridgeAR commented Mar 9, 2019

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

@BridgeAR BridgeAR mentioned this pull request Mar 10, 2019
Closed
3 tasks
@BridgeAR
Copy link
Copy Markdown
Member Author

BridgeAR commented Mar 10, 2019 •
edited
Loading

Resumed CI https://ci.nodejs.org/job/node-test-commit/26577/ ✔️

@BridgeAR
Copy link
Copy Markdown
Member Author

BridgeAR commented Mar 12, 2019

It would be nice to get some further reviews @nodejs/repl @nodejs/util

Fishrock123
Fishrock123 approved these changes Mar 18, 2019
View reviewed changes
Comment thread lib/internal/tty.js
case '':
case '1':
case 'true':
warnOnDeactivatedColors(env);
Copy link
Copy Markdown
Contributor

@Fishrock123 Fishrock123 Mar 18, 2019

Choose a reason for hiding this comment

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

Place this above the switch?

Copy link
Copy Markdown
Member Author

@BridgeAR BridgeAR Mar 18, 2019

Choose a reason for hiding this comment

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

When moved above the switch, I'll have to exclude a lot of values. That's why it's inside.

BridgeAR added a commit to BridgeAR/node that referenced this pull request Mar 19, 2019
@BridgeAR
tty: add NO_COLOR and FORCE_COLOR support
273398a 
This adds support to enforce a specific color depth by checking the
`FORCE_COLOR` environment variable similar to `chalk`.

On top of that we also add support for the `NO_COLOR` environment
variable as suggested by https://no-color.org/.

PR-URL: nodejs#26485
Refs: nodejs#26248
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@BridgeAR
Copy link
Copy Markdown
Member Author

BridgeAR commented Mar 19, 2019

Landed in 273398a 🎉

@BridgeAR BridgeAR closed this Mar 19, 2019
targos pushed a commit to targos/node that referenced this pull request Mar 27, 2019
@BridgeAR @targos
tty: add NO_COLOR and FORCE_COLOR support
d6bff81 
This adds support to enforce a specific color depth by checking the
`FORCE_COLOR` environment variable similar to `chalk`.

On top of that we also add support for the `NO_COLOR` environment
variable as suggested by https://no-color.org/.

PR-URL: nodejs#26485
Refs: nodejs#26248
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
targos pushed a commit that referenced this pull request Mar 27, 2019
@BridgeAR @targos
tty: add NO_COLOR and FORCE_COLOR support
8cafd83 
This adds support to enforce a specific color depth by checking the
`FORCE_COLOR` environment variable similar to `chalk`.

On top of that we also add support for the `NO_COLOR` environment
variable as suggested by https://no-color.org/.

PR-URL: #26485
Refs: #26248
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
targos added a commit that referenced this pull request Mar 27, 2019
@targos
2019-03-28, Version 11.13.0 (Current)
04903d8 
Notable changes:

* events:
  * Added a `once` function to use `EventEmitter` with promises
    (#26078).
* tty:
  * Added a `hasColors` method to `WriteStream`
    (#26247).
  * Added NO_COLOR and FORCE_COLOR support
    (#26485).
* v8:
  * Added `v8.getHeapSnapshot` and `v8.writeHeapSnapshot` to generate snapshots
    in the format used by tools such as Chrome DevTools
    (#26501).
* meta:
  * Gireesh Punathil is now a member of the Technical Steering Committee
    (#26657).
  * Added ZYSzys to collaborators (#26730).

PR-URL: #26949
@targos targos mentioned this pull request Mar 27, 2019
Merged
targos added a commit that referenced this pull request Mar 28, 2019
@targos
2019-03-28, Version 11.13.0 (Current)
29867f3 
Notable changes:

* crypto
  * Allow deriving public from private keys (Tobias Nießen)
    [#26278](#26278).
* events
  * Added a `once` function to use `EventEmitter` with promises
    (Matteo Collina) [#26078](#26078).
* tty
  * Added a `hasColors` method to `WriteStream` (Ruben Bridgewater)
    [#26247](#26247).
  * Added NO_COLOR and FORCE_COLOR support (Ruben Bridgewater)
    [#26485](#26485).
* v8
  * Added `v8.getHeapSnapshot` and `v8.writeHeapSnapshot` to generate snapshots
    in the format used by tools such as Chrome DevTools (James M Snell)
    [#26501](#26501).
* worker
  * Added `worker.moveMessagePortToContext`. This enables using MessagePorts in
    different vm.Contexts, aiding with the isolation that the vm module seeks to
    provide (Anna Henningsen)
    [#26497](#26497).
* C++ API
  * `AddPromiseHook` is now deprecated. This API was added to fill an use case
    that is served by `async_hooks`, since that has `Promise` support
    (Anna Henningsen) [#26529](#26529).
  * Added a `Stop` API to shut down Node.js while it is running
    (Gireesh Punathil) [#21283](#21283).
* meta
  * [Gireesh Punathil](https://github.com/gireeshpunathil) is now a member of
    the Technical Steering Committee
    [#26657](#26657).
  * Added [Yongsheng Zhang](https://github.com/ZYSzys) to collaborators
    [#26730](#26730).

PR-URL: #26949
targos added a commit that referenced this pull request Mar 28, 2019
@targos
2019-03-28, Version 11.13.0 (Current)
2c73868 
Notable changes:

* crypto
  * Allow deriving public from private keys (Tobias Nießen)
    [#26278](#26278).
* events
  * Added a `once` function to use `EventEmitter` with promises
    (Matteo Collina) [#26078](#26078).
* tty
  * Added a `hasColors` method to `WriteStream` (Ruben Bridgewater)
    [#26247](#26247).
  * Added NO_COLOR and FORCE_COLOR support (Ruben Bridgewater)
    [#26485](#26485).
* v8
  * Added `v8.getHeapSnapshot` and `v8.writeHeapSnapshot` to generate snapshots
    in the format used by tools such as Chrome DevTools (James M Snell)
    [#26501](#26501).
* worker
  * Added `worker.moveMessagePortToContext`. This enables using MessagePorts in
    different vm.Contexts, aiding with the isolation that the vm module seeks to
    provide (Anna Henningsen)
    [#26497](#26497).
* C++ API
  * `AddPromiseHook` is now deprecated. This API was added to fill an use case
    that is served by `async_hooks`, since that has `Promise` support
    (Anna Henningsen) [#26529](#26529).
  * Added a `Stop` API to shut down Node.js while it is running
    (Gireesh Punathil) [#21283](#21283).
* meta
  * [Gireesh Punathil](https://github.com/gireeshpunathil) is now a member of
    the Technical Steering Committee
    [#26657](#26657).
  * Added [Yongsheng Zhang](https://github.com/ZYSzys) to collaborators
    [#26730](#26730).

PR-URL: #26949
BethGriggs pushed a commit that referenced this pull request Apr 5, 2019
@targos @BethGriggs
2019-03-28, Version 11.13.0 (Current)
473b162 
Notable changes:

* crypto
  * Allow deriving public from private keys (Tobias Nießen)
    [#26278](#26278).
* events
  * Added a `once` function to use `EventEmitter` with promises
    (Matteo Collina) [#26078](#26078).
* tty
  * Added a `hasColors` method to `WriteStream` (Ruben Bridgewater)
    [#26247](#26247).
  * Added NO_COLOR and FORCE_COLOR support (Ruben Bridgewater)
    [#26485](#26485).
* v8
  * Added `v8.getHeapSnapshot` and `v8.writeHeapSnapshot` to generate snapshots
    in the format used by tools such as Chrome DevTools (James M Snell)
    [#26501](#26501).
* worker
  * Added `worker.moveMessagePortToContext`. This enables using MessagePorts in
    different vm.Contexts, aiding with the isolation that the vm module seeks to
    provide (Anna Henningsen)
    [#26497](#26497).
* C++ API
  * `AddPromiseHook` is now deprecated. This API was added to fill an use case
    that is served by `async_hooks`, since that has `Promise` support
    (Anna Henningsen) [#26529](#26529).
  * Added a `Stop` API to shut down Node.js while it is running
    (Gireesh Punathil) [#21283](#21283).
* meta
  * [Gireesh Punathil](https://github.com/gireeshpunathil) is now a member of
    the Technical Steering Committee
    [#26657](#26657).
  * Added [Yongsheng Zhang](https://github.com/ZYSzys) to collaborators
    [#26730](#26730).

PR-URL: #26949
@Qix-
Copy link
Copy Markdown

Qix- commented Aug 27, 2019 •
edited
Loading

@BridgeAR BridgeAR deleted the add-force-color branch January 20, 2020 11:55
@chocolateboy chocolateboy mentioned this pull request Sep 26, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Trott Trott Awaiting requested review from Trott

@targos targos Awaiting requested review from targos

@starkwang starkwang Awaiting requested review from starkwang

@addaleax addaleax Awaiting requested review from addaleax

@mcollina mcollina Awaiting requested review from mcollina

@mhdawson mhdawson Awaiting requested review from mhdawson

@danbev danbev Awaiting requested review from danbev

@jasnell jasnell Awaiting requested review from jasnell

@jdalton jdalton Awaiting requested review from jdalton

@lance lance Awaiting requested review from lance

3 more reviewers

@vsemozhetbyt vsemozhetbyt vsemozhetbyt left review comments

@silverwind silverwind silverwind approved these changes

@Fishrock123 Fishrock123 Fishrock123 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. semver-minor PRs that contain new features and should be released in the next minor version. tty Issues and PRs related to the tty subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@BridgeAR @nodejs-github-bot @Qix- @silverwind @Fishrock123 @vsemozhetbyt @BethGriggs