Open medikoo opened 2 years ago
First things first, from reading the documentation on bl
I can't see any way it replaces mute-stream
nor do I see any way we tweak it in our code so it does what we want it to do. We probably need some input from either @sindresorhus or @aminya on that.
ourEmit
really seems dead, and was dead since my original https://github.com/sindresorhus/ora/commit/a4b225352329b68aa871c968f79f4f61191e29d3 . The path to make the whole thing back then was quite painful, so it's either me forgetting to delete the code along the way, or me forgetting to turn it back on(in which case it's supposed to fix some edge case that isn't currently fixed). Need some further investigation and experimenting to be sure.
Re: SIGINT
stuff:
The idea here is to trigger native handling code(internal code path have some undocumented intricacies and seems to change relatively frequently) if there are no listeners(so the user expects SIGINT
to be handled as per usual), but do our best to not screw things up completely when listeners are trying to handle stuff(we don't define the exact behaviour in that case, just try our best and wait for bug reports in case we failed). So the condition seems ok to me, though we might want to add some comments.
if there are no listeners(so the user expects SIGINT to be handled as per usual),
By default Node.js if there are no listeners, exits the process. While in ora default is to simply re-emit the event to no listeners (as it's confirmed there are no listeners on event).
So logically this breaks the contract. It doesn't turn into real issue only because with ora there's always other listeners on SIGINT
(due to signal-exit
being loaded indirectly)
It doesn't turn into a real issue only ..
Actually, I take it back, it introduces an issue, as it unconditionally kills the process, not letting SIGINT
listeners to handle the issue. If ora
would be used in the process that registers some cleanup on SIGINT
, it won't be run
@medikoo Ok, rereading the code I kinda feel that you're right. However, if I run a test, the code works exactly as intended the way it is now. With no listeners on SIGINT
, it just exits properly, leaving the terminal in a proper state. When there is a listener on SIGINT
it doesn't exit and gives control to the listener(Though prints ^C
, which isn't supposed to happen when we use discardStdin
.) Am I going crazy?
import ora from 'ora';
const spinner = ora({
text: 'Not catching SIGINT',
discardStdin: true,
}).start();
setTimeout(() => {
let sigintsCaught = 0;
spinner.text = 'Catching SIGINT';
process.on('SIGINT', () => {
sigintsCaught++;
console.log('sigints caught', sigintsCaught);
if (sigintsCaught === 5) {
process.exit(0);
}
});
}, 3000);
@stroncium discardStdin
option in ora
doesn't work at all (it's why I've opened this issue), and similarly the SIGINT
listener at https://github.com/sindresorhus/ora/blob/c5026b7b411765636ae08a8f4a15789617ba695e/index.js#L76 doesn't pick SIGINT
from stdin.
What you observe is same behavior as if you'd pass discardStdin: false
.
discardStdin
definitely does something, including instantiating readline
. readline
stopped emitting SIGINTs it seems, probably need a bug report on Node.js for that part.
Regarding this issue, it's getting convoluted, I propose to deal with the first part(switch to bl
and not actually discarding stdin) as it limits further actions regarding SIGINTs and deal with the rest in follow up.
@stroncium Hi, is there any progress? I also noticed discardStdin
option is not working. I am testing on M1 Mac.
As I test in latest installation of ora, pressing enter creates a new line and duplicates a progress line, which gives impression that discard of stdin is not effective.
As I investigated it stopped working after https://github.com/sindresorhus/ora/pull/163 was taken.
Additional logical issues I've spotted in codebase:
ourEmit
looks as dead code. this function seem to be never used (do I miss something?)SINGINT
handling condition seems logically incorrect, however it's not an issue, asora
loadingsignal-exit
(through its dependencies) ensures there's always some listener toSIGINT
event attached. Nonetheless condition should start withif (process.listenerCount('SIGINT')) {
and notif (process.listenerCount('SIGINT') === 0) {
I'm not proposing a PR, as I'm not sure how conversion to
bl
should be handled (whether implementation should stick to it, but fix its setup, or revert tomute-stream
where it worked)/cc @aminya @stroncium