-
Notifications
You must be signed in to change notification settings - Fork 54
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
Implement more robust isTTY check #63
Conversation
@jorgebucaran What's up with GitHub actions? I see there is a CI flow defined, but it doesn't seem to be running. |
I can confirm this fixes the compatibility problems we have for pino transport |
Wow, thanks! So, is this Node's canonical way to detect if you are in a pipe? Other than not being available inside worker_threads, are there any disadvantages to |
It was configured to run against push only. It's fixed now. 🙆♂️ |
@jorgebucaran This discussion might be useful: nodejs/node#28386 |
@jorgebucaran is there an ETA for a release that would ship this? |
index.js
Outdated
@@ -1,12 +1,13 @@ | |||
const tty = require('tty'); |
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.
Colorette is an ES module, so this should be changed to use import
.
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.
will that be compatible with current dual publishing of CJS and ES versions? do you autoreplace?
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.
Nope, looks like the time to upgrade that script has finally come! I can take care of that after we merge this.
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.
done
Thanks! I'll merge here, update the script and publish a new minor version shortly. |
Released as 1.4.0. |
@mcollina @kibertoad Is there any reason to check if import * as tty from "tty"
...
const isCompatibleTerminal =
tty && tty.isatty(1) && env.TERM && env.TERM !== "dumb" |
@jorgebucaran It's a node-specific thing. There were prior bug reports from people using colorette outside of Node, I wanted to safeguard against that from the get go. |
Gotcha, thanks. I still feel that this should be handled by the user's bundler / preprocessor (#64 (comment)). |
As per pinojs/pino-pretty#220 (comment)