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

Add basic color detection #5

Merged
merged 15 commits into from
Oct 27, 2021
Merged

Add basic color detection #5

merged 15 commits into from
Oct 27, 2021

Conversation

Richienb
Copy link
Contributor

@Richienb Richienb commented Oct 8, 2021

No description provided.

@Richienb Richienb marked this pull request as ready for review October 8, 2021 01:52
sindresorhus
sindresorhus reviewed Oct 8, 2021
View reviewed changes
readme.md Outdated Show resolved Hide resolved
@Richienb Richienb requested a review from sindresorhus October 8, 2021 06:47
@sindresorhus
Copy link
Owner

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

@sindresorhus
Copy link
Owner

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
Copy link
Contributor Author

Richienb commented Oct 9, 2021 •
edited
Loading

@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
Copy link
Owner

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

I think that should be done regardless.

@sindresorhus
Copy link
Owner

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

@Richienb Richienb mentioned this pull request Oct 9, 2021
Closed
@sindresorhus
Copy link
Owner

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
Copy link
Owner

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
Copy link
Contributor Author

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

targos pushed a commit to nodejs/node that referenced this pull request Oct 23, 2021
@Richienb @targos
tty: support more CI services in getColorDepth
f194082 
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>
targos pushed a commit to nodejs/node that referenced this pull request Oct 23, 2021
@Richienb @targos
tty: support more CI services in getColorDepth
a95e344 
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
Copy link
Owner

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
Copy link
Contributor Author

Config errors caused the build to fail - fixed now.

@sindresorhus sindresorhus merged commit 2aa7cec into sindresorhus:main Oct 27, 2021
kirillgroshkov added a commit to NaturalCycles/nodejs-lib that referenced this pull request Oct 31, 2021
@kirillgroshkov
feat: change/simplify hasColors implementation
13f32d1 
It is now a constant, not a function. Dependency-free detection.

Credit to sindresorhus/yoctocolors#5
BethGriggs pushed a commit to nodejs/node that referenced this pull request Nov 24, 2021
@Richienb @BethGriggs
tty: support more CI services in getColorDepth
7d550ad 
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
privatenumber reviewed Oct 19, 2022
View reviewed changes
index.js
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();
Copy link

Choose a reason for hiding this comment

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

@Richienb

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't remember.

@sindresorhus

Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

Sorry, I missed that.

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@privatenumber privatenumber privatenumber left review comments

@sindresorhus sindresorhus Awaiting requested review from sindresorhus

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Richienb @sindresorhus @privatenumber