-
Notifications
You must be signed in to change notification settings - Fork 448
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.
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
fixed support-colors condition orders, this should fix cygwin issues #154
Conversation
This fixes colors for Linux as well and for git for windows (bash). |
return false; | ||
} | ||
|
||
if (/^screen|^xterm|^vt100|color|ansi|cygwin|linux/i.test(process.env.TERM)) { | ||
return true; | ||
if (process.env.TERM === 'dumb') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switching the dumb
check and the other process.env.TERM
check does not change anything, because they are exclusive. All it does it making this diff harder to read. I suggest you switch them back to minimize this diff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this was done on purpose since
if (/^screen|^xterm|^vt100|color|ansi|cygwin|linux/i.test(process.env.TERM)) {
is moved to below if ('COLORTERM' in process.env) {
and moved ontop of
if (process.stdout && !process.stdout.isTTY) {
But this change does something since it now works in windows, and Linux.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The process.stdout
check should not have been moved, see my other comment above. When you move it back, the remaining change is that the two process.env.TERM
switch place, which does nothing. Which means there is nothing left this patch does right. Sorry to say that.
@paladox The reason this patch seems to fix it for you is because it returns true for For example, even if your terminal supports colors, if you're sending the output of your program to another program or to a file (so not to the terminal screen), then we typically expect plain text (without color codes). While this patch it enables colors in interactive terminals on Windows for you; it also enables colors everywhere else - which defeats the purpose of the |
I think we should close this in favor of updating supports-colors from the official supports-colors repo ( https://github.com/chalk/supports-color/blob/master/index.js ). That should resolve a lot of these issues. Planning that for a (soon) future release. |
Support-colors.js's condition order was a mess.
Changed to make it fit to logical rules, instead of randomness.