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

feat(common): do not use colors in CLI if not supported #9278

Merged
merged 4 commits into from
Mar 14, 2022
Merged

feat(common): do not use colors in CLI if not supported #9278

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ec9537b
feat(common): do not use color in CLI if not supported
jonahsnider Mar 1, 2022
3147279
fix(common): fix condition
jonahsnider Mar 1, 2022
4e7dd9a
style(common): format
jonahsnider Mar 1, 2022
07c9ce7
style(common): format
jonahsnider Mar 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion packages/common/utils/cli-colors.util.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import {WriteStream} from 'tty';

type ColorTextFn = (text: string) => string;

const isColorAllowed = () => !process.env.NO_COLOR;
const isColorAllowed = () => !process.env.NO_COLOR && WriteStream.prototype.hasColors();
Copy link
Member

Choose a reason for hiding this comment

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

This is mostly only used by the ConsoleLogger, but there are references to it in the Injector.ts file as well. If it was just the ConsoleLogger I'd say this is okay, or even checking process.stdout.hasColors() directly instead of needing the prototype, but I'm not sure how to handle if this check is coming from the Injector.ts

Overall, I don't think this should be an issue, my only concern would be if someone is in an environment where hasColors() returns false but they want to force the use of colors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

even checking process.stdout.hasColors() directly instead of needing the prototype

That only checks stdout and not stderr. See nodejs/node#40236 and nodejs/node#40240 for discussion on exposing a better way to access Writestream.prototype.hasColors().

but I'm not sure how to handle if this check is coming from the Injector.ts

isColorAllowed() could be refactored to allow an optional parameter of the stream that is being written to and call .hasColors() on that directly.

Overall, I don't think this should be an issue, my only concern would be if someone is in an environment where hasColors() returns false but they want to force the use of colors.

They can use the FORCE_COLOR environment variable.

const colorIfAllowed = (colorFn: ColorTextFn) => (text: string) =>
isColorAllowed() ? colorFn(text) : text;

Expand Down