fix: clear output using stream methods

[43081j]

Clears the output using the stream methods and changes the logic slightly to clear per line, other than line 0.

@PondWader I'm trying to fix this:

  const finalSpinner = new Spinner('Attempting risky operation...');
  finalSpinner.start();
  try {
    await new Promise((resolve) => setTimeout(resolve, 2000));
    throw new Error('Intentional error');
  } catch (error) {
    console.error('\nError caught:', error.message);
  } finally {
    finalSpinner.stop();
  }

basically, this currently ends up clearing the console.error when we stop the spinner

I'm not 100% sure why, but this change seems to fix that...

I'm leaving this as a draft until we can understand what's going on here. if you could take a look, that'd be super helpful

[PondWader]

Generally the expectation is that nothing should be written to stdout/stderr whilst the spinner is running. ora doesn't handle this either.

I don't believe there is a proper way to handle it, it looks like your solution just skips clearing the first line but this won't work if multiple lines are output. Ideally if someone wants to output something whilst the spinner is running they should use the Renderer instance which is exported.

[43081j]

This particular case does seem to output the warning in ora and yocto

I agree I would have expected all output while the spinner spins, to be ignored

We have some projects that are trying to migrate but have cases like this where error output is being eaten up

cc @JamesHenry maybe you can explain better

[PondWader]

The reason for that is because ora doesn't output a new line after the spinner so it only clears one line instead of two. We could change picospinner to do that.

If the user's terminal is sufficiently small or the text is too long to fit on one line, ora will clear the text.

As I said, picospinner does provide an official way (although undocumented) to display an output while the spinner is running which I don't believe ora does.

[43081j]

Makes sense

So it sounds like we do the right thing (but maybe could still benefit from using the helper methods)

And consumers should just be aware that output should happen after stopping the spinner

Let's see what James thinks too as he's one of the users that raised this. So will be good to know if it's a blocker or anything

[JamesHenry]

Maybe I'm not following correctly but this discussion doesn't match my reproduction:

https://stackblitz.com/edit/stackblitz-starters-dmnx9n?file=package.json&view=editor

node ora.js 

node picospinner.js 

node nanospinner.js 

ora does handle the error handling (printing) as expected? picospinner and nanospinner do not, with nanospinner double printing, and picospinner exiting without printing

[PondWader]

Ora is not handling it as such. It's because ora doesn't emit a new line after the spinner so it only tries to remove one line. If you use a multi-line spinner it won't work.

console.log and console.error emit new lines after the string they print, ora tries to remove a single line in your example so only removes that new line.

[43081j]

yep so @JamesHenry I think picospinner is actually doing the right thing here, in that all output while a spinner is spinning should be cleared

meanwhile, ora has a bug which means this doesn't happen

but people could be relying on that, so the difference in behaviour isn't ideal

if we leave things as they are, it means your pattern would be more like this:

try {
  doSomething();

  spinner.stop();
} catch (err) {
  spinner.stop();
  console.log(err);
}

this is more correct since, in all 3 libraries, we should be stopping the spinner before outputting things

though it isn't as clean looking since we have to call stop in two places rather than using a finally

alternatively, we could purposely break picospinner to behave the same way as ora, but that doesn't feel right either

@PondWader is also right, if you set the message as risky\noperation, both ora and picospinner clear the error

what are your thoughts?

[PondWader]

It's not so much a bug in ora, just their spinner takes up only one line. Displaying a message before stopping the spinner is using unreliable behaviour though (what if the user's terminal is small enough that the spinner text has to go on to 2 lines?).

picospinner does provide a proper way to display text whilst the spinner is still running but if you're about to stop the spinner anyway it's a bit unnecesary to use it.

[JamesHenry]

Ok fair enough, thanks folks 👍

However, I added a new file to my stackblitz called unhandled: https://stackblitz.com/edit/stackblitz-starters-dmnx9n?file=unhandled.js&view=editor

It checks how each library behaves when an unhandled exception occurs.

I think nanospinner is pretty clearly the best here, the other two suppress the error entirely, with picospinner removing all trace of the spinner as well so you have even less of a sense of what happened when/where after the fact

[43081j]

@PondWader any idea what causes that?

I can't imagine nanospinner is doing anything special for unhandled exceptions. So it's probably luckily not clearing it?

We would need to catch exceptions, stop, then rethrow if we wanted this to happen on purpose right?

[PondWader]

@PondWader any idea what causes that?

My guess is that they don't listen for the "exit" event and clean up the spinner afterwards.