Add basic color detection by Richienb · Pull Request #5 · sindresorhus/yoctocolors

Richienb · 2021-10-08T01:48:26Z

No description provided.

sindresorhus · 2021-10-08T06:57:45Z

Sorry, I was unclear. The todo comment should be in the code, not readme.

sindresorhus · 2021-10-08T13:39:47Z

Tests are failing.

And thinking more about this, I think we should document the supported flags & env variables (FORCE_COLOR and NO_COLOR), since the Node.js docs seem to not do that: https://github.com/nodejs/node/blob/69b6d1d17b90892ec4aceb2136133d1f44646771/lib/internal/tty.js

Richienb · 2021-10-09T01:19:21Z

@sindresorhus The build is failing because Node.js is not detecting features on the GitHub Actions terminal that indicate color support. See the related supports-color issue. The best solution might be to sync the Node.js implementation with the fixes.

sindresorhus · 2021-10-09T05:24:54Z

The best solution might be to sync the Node.js implementation with the fixes.

I think that should be done regardless.

sindresorhus · 2021-10-09T05:25:20Z

I think a better fix would be to use FORCE_COLOR as it would work in other environments too, like when using np (since it spawns).

sindresorhus · 2021-10-11T07:14:51Z

And thinking more about this, I think we should document the supported flags & env variables (FORCE_COLOR and NO_COLOR), since the Node.js docs seem to not do that: nodejs/node@69b6d1d/lib/internal/tty.js

Did you see ^?

sindresorhus · 2021-10-11T07:15:06Z

And thinking more about this, I think we should document the supported flags & env variables (FORCE_COLOR and NO_COLOR), since the Node.js docs seem to not do that: nodejs/node@69b6d1d/lib/internal/tty.js

Maybe also worth doing a Node.js PR to document these things?

Richienb · 2021-10-20T12:01:55Z

@sindresorhus FORCE_COLOR, NO_COLOR and NODE_DISABLE_COLORS are already documented. Is it missing something else?

Also, this PR should be good to go.

PR-URL: #40385 Refs: sindresorhus/yoctocolors#5 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

sindresorhus · 2021-10-24T00:26:17Z

FORCE_COLOR, NO_COLOR and NODE_DISABLE_COLORS are already documented. Is it missing something else?

That's not the docs we are linking to in the readme though. And I think it's good to just document it here anyway for ease of discovery.

Richienb · 2021-10-27T12:00:28Z

Config errors caused the build to fail - fixed now.

It is now a constant, not a function. Dependency-free detection. Credit to sindresorhus/yoctocolors#5

PR-URL: #40385 Refs: sindresorhus/yoctocolors#5 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

privatenumber · 2022-10-19T13:39:07Z

+import tty from 'node:tty';
+
+// TODO: Use a better method when it's added to Node.js (https://github.com/nodejs/node/pull/40240)
+const hasColors = tty.WriteStream.prototype.hasColors();


@Richienb

Was there a reason for doing this instead of process.stdout.hasColors()?

I don't remember.

@sindresorhus

I believe the answer is in the TODO comment. process.stdout.hasColors() was not available then.

Sorry, I missed that.

Seems process.stdout.hasColors() has been available. However, they're trying to deprecate it because:

it's based on the environment rather than the stream

it's not available when process.stdout is not a proper TTY

In the PR, they're trying to expose a standalone function but is blocked by deprecation strategy for the prototype method.

Richienb added 4 commits October 8, 2021 14:46

Add basic color detection

511abb0

Update readme.md

644ac8f

Update readme.md

9aacd55

Update readme.md

d49a990

Richienb marked this pull request as ready for review October 8, 2021 01:52

sindresorhus reviewed Oct 8, 2021

View reviewed changes

Comment thread readme.md Outdated

Update readme.md

3536e7a

Richienb requested a review from sindresorhus October 8, 2021 06:47

Richienb added 2 commits October 9, 2021 00:02

Update readme.md

c9a0a58

Update index.js

bb3464a

Richienb mentioned this pull request Oct 9, 2021

tty: support more CI services in getColorDepth nodejs/node#40385

Closed

Update package.json

b95ad78

sindresorhus and others added 6 commits October 24, 2021 07:27

Update readme.md

aa3a93c

Update readme.md

42368f0

Update readme.md

2a5d748

Update package.json

47155ea

Update package.json

23f0018

Update package.json

67ff9d8

Update readme.md

62937bf

sindresorhus merged commit 2aa7cec into sindresorhus:main Oct 27, 2021

kirillgroshkov added a commit to kirillgroshkov/nodejs-lib that referenced this pull request Oct 31, 2021

feat: change/simplify hasColors implementation

13f32d1

It is now a constant, not a function. Dependency-free detection. Credit to sindresorhus/yoctocolors#5

privatenumber reviewed Oct 19, 2022

View reviewed changes

Uh oh!

Conversation

Richienb commented Oct 8, 2021

Uh oh!

Uh oh!

sindresorhus commented Oct 8, 2021

Uh oh!

sindresorhus commented Oct 8, 2021

Uh oh!

Richienb commented Oct 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sindresorhus commented Oct 9, 2021

Uh oh!

sindresorhus commented Oct 9, 2021

Uh oh!

sindresorhus commented Oct 11, 2021

Uh oh!

sindresorhus commented Oct 11, 2021

Uh oh!

Richienb commented Oct 20, 2021

Uh oh!

sindresorhus commented Oct 24, 2021

Uh oh!

Richienb commented Oct 27, 2021

Uh oh!

privatenumber Oct 19, 2022

Choose a reason for hiding this comment

Uh oh!

Richienb Oct 19, 2022

Choose a reason for hiding this comment

Uh oh!

sindresorhus Oct 20, 2022

Choose a reason for hiding this comment

Uh oh!

privatenumber Oct 20, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Richienb commented Oct 9, 2021 •

edited

Loading