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

fixed support-colors condition orders, this should fix cygwin issues #154

Closed
wants to merge 1 commit into from
Closed

fixed support-colors condition orders, this should fix cygwin issues #154

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
14 changes: 7 additions & 7 deletions lib/system/supports-colors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,6 @@ module.exports = (function () {
return true;
}

if (process.stdout && !process.stdout.isTTY) {
return false;
}

if (process.platform === 'win32') {
Copy link

Choose a reason for hiding this comment

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

Moving this check below all the others means this will create messy text files when redirecting the output to a text file via >output.txt. If this happens will then depend on the platform. win32 for example will have this problem, other platforms may not, depending on the order of the checks below.

return true;
}
Expand All @@ -49,12 +45,16 @@ module.exports = (function () {
return true;
}

if (process.env.TERM === 'dumb') {
if (/^screen|^xterm|^vt100|color|ansi|cygwin|linux/i.test(process.env.TERM)) {
return true;
}

if (process.stdout && !process.stdout.isTTY) {
return false;
}

if (/^screen|^xterm|^vt100|color|ansi|cygwin|linux/i.test(process.env.TERM)) {
return true;
if (process.env.TERM === 'dumb') {
Copy link

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.

Copy link
Contributor

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.

Copy link

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.

return false;
Copy link

Choose a reason for hiding this comment

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

This last check could not be omitted because it is the default anyway.

}

return false;
Expand Down