Discard of stdin doesn't seem to work

[medikoo]

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 #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, as ora loading signal-exit (through its dependencies) ensures there's always some listener to SIGINT event attached. Nonetheless condition should start with if (process.listenerCount('SIGINT')) { and not if (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 to mute-stream where it worked)

/cc @aminya @stroncium

[stroncium]

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 a4b2253 . 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.

[medikoo]

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)

[medikoo]

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

[stroncium]

@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);

[medikoo]

@stroncium discardStdin option in ora doesn't work at all (it's why I've opened this issue), and similarly the SIGINT listener at

ora/index.js

Line 76 in c5026b7

this.rl.on('SIGINT', () => {

doesn't pick SIGINT from stdin.

What you observe is same behavior as if you'd pass discardStdin: false.

[stroncium]

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.

[kejiweixun]

@stroncium Hi, is there any progress? I also noticed discardStdin option is not working. I am testing on M1 Mac.