-
Notifications
You must be signed in to change notification settings - Fork 149
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
--no-color
fix
#5207
--no-color
fix
#5207
Conversation
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.
Tophatted and LGTM!
Coverage report
Show files with reduced coverage 🔻
Test suite run success2003 tests passing in 905 suites. Report generated by 🧪jest coverage report action from 3693514 |
ed0047f
to
2793fa7
Compare
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.
Other than the eslint errors that need fixing this looks good!
Certain versions of node add colors to console.warn and console.error which doesn't respect our control of colors via chalk. To avoid this kind of interference with the CLI, we should write directly to output streams.
2793fa7
to
3693514
Compare
WHY are these changes introduced?
The user's expressed
--no-color
preference is not respected when logging warnings and errors.Additional context: Some versions of node color
console.warn
output yellow andconsole.error
output red unless they output is already colored explicitly. While this has been rolled back, it seems safest to stop relying on these functions and simply write directly to stdout/stderr.WHAT is this pull request doing?
$ p shopify kitchen-sink static --no-color
Before:
After:
How to test your changes?
$ p shopify kitchen-sink static
with and without--no-color
Post-release steps
Measuring impact
How do we know this change was effective? Please choose one:
Checklist