tty: add NO_COLOR and FORCE_COLOR support #26485

BridgeAR · 2019-03-07T01:15:05Z

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 · 2019-03-07T01:15:07Z

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

BridgeAR · 2019-03-07T01:15:56Z

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

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/.

silverwind

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

BridgeAR · 2019-03-08T15:22:00Z

@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.

vsemozhetbyt

Doc format LGTM with nits.

doc/api/tty.md

BridgeAR · 2019-03-09T22:37:35Z

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

BridgeAR · 2019-03-10T20:41:58Z

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

BridgeAR · 2019-03-12T17:32:57Z

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

Fishrock123 · 2019-03-18T23:12:30Z

lib/internal/tty.js

+      case '':
+      case '1':
+      case 'true':
+        warnOnDeactivatedColors(env);


Place this above the switch?

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

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 · 2019-03-19T15:43:19Z

Landed in 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>

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>

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

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- · 2019-08-27T10:25:37Z

Really wish someone from the Chalk team was pinged about this. We had a lot of discussion about these environment variables.

There was a strong resistance against having two of them - the semantics are unclear and arbitrary when both FORCE_COLOR=1 and NO_COLOR=1 are defined. Which one takes precedence? It's entirely up to the implementation.

By instead using FORCE_COLOR=0, you remove any confusion. None of this was even questioned before being blindly merged and set in stone in Node.js other than @silverwind suggesting it.

This is only adding to confusion and working in the opposite direction of efforts being made by terminal emulator implementors trying to cooperatively simplify this whole mess of terminal styling and colors.

cc @sindresorhus

BridgeAR requested review from Trott, targos, vsemozhetbyt and starkwang March 7, 2019 01:15

nodejs-github-bot added the tty Issues and PRs related to the tty subsystem. label Mar 7, 2019

BridgeAR mentioned this pull request Mar 7, 2019

doc: add examples for tty.getColorDepth() env #26248

Closed

4 tasks

BridgeAR requested a review from addaleax March 7, 2019 01:15

BridgeAR added the semver-minor PRs that contain new features and should be released in the next minor version. label Mar 7, 2019

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 force-pushed the add-force-color branch from b1653fb to af0b138 Compare March 7, 2019 07:31

BridgeAR mentioned this pull request Mar 7, 2019

Add Jest to lookup nodejs/citgm#684

Closed

BridgeAR requested review from mcollina, mhdawson and silverwind March 8, 2019 14:49

silverwind approved these changes Mar 8, 2019

View reviewed changes

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 reviewed Mar 8, 2019

View reviewed changes

doc/api/tty.md Outdated Show resolved Hide resolved

doc/api/tty.md Outdated Show resolved Hide resolved

doc/api/tty.md Outdated Show resolved Hide resolved

doc/api/tty.md Outdated Show resolved Hide resolved

fixup! tty: add NO_COLOR and FORCE_COLOR support

8708e7e

BridgeAR mentioned this pull request Mar 10, 2019

lib: support dumb terminals #26261

Closed

3 tasks

BridgeAR requested review from danbev, jasnell, jdalton and lance March 15, 2019 17:10

Fishrock123 approved these changes Mar 18, 2019

View reviewed changes

BridgeAR closed this Mar 19, 2019

targos mentioned this pull request Mar 27, 2019

v11.13.0 release proposal #26949

Merged

BethGriggs added the lts-watch-v10.x label Aug 20, 2019

BethGriggs added dont-land-on-v10.x and removed lts-watch-v10.x labels Sep 3, 2019

BridgeAR deleted the add-force-color branch January 20, 2020 11:55

chocolateboy mentioned this pull request Sep 26, 2020

Fix NO_COLOR handling to comply with the spec lukeed/kleur#40

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tty: add NO_COLOR and FORCE_COLOR support #26485

tty: add NO_COLOR and FORCE_COLOR support #26485

BridgeAR commented Mar 7, 2019

nodejs-github-bot commented Mar 7, 2019

BridgeAR commented Mar 7, 2019 •

edited

Loading

silverwind left a comment

BridgeAR commented Mar 8, 2019

vsemozhetbyt left a comment

BridgeAR commented Mar 9, 2019

BridgeAR commented Mar 10, 2019 •

edited

Loading

BridgeAR commented Mar 12, 2019

Fishrock123 Mar 18, 2019

BridgeAR Mar 18, 2019

BridgeAR commented Mar 19, 2019

Qix- commented Aug 27, 2019 •

edited

Loading

tty: add NO_COLOR and FORCE_COLOR support #26485

tty: add NO_COLOR and FORCE_COLOR support #26485

Conversation

BridgeAR commented Mar 7, 2019

Checklist

nodejs-github-bot commented Mar 7, 2019

BridgeAR commented Mar 7, 2019 • edited Loading

silverwind left a comment

Choose a reason for hiding this comment

BridgeAR commented Mar 8, 2019

vsemozhetbyt left a comment

Choose a reason for hiding this comment

BridgeAR commented Mar 9, 2019

BridgeAR commented Mar 10, 2019 • edited Loading

BridgeAR commented Mar 12, 2019

Fishrock123 Mar 18, 2019

Choose a reason for hiding this comment

BridgeAR Mar 18, 2019

Choose a reason for hiding this comment

BridgeAR commented Mar 19, 2019

Qix- commented Aug 27, 2019 • edited Loading

BridgeAR commented Mar 7, 2019 •

edited

Loading

BridgeAR commented Mar 10, 2019 •

edited

Loading

Qix- commented Aug 27, 2019 •

edited

Loading