Fix setRawMode with non-TTY stdin and expose isRawModeSupported on StdinContext

[eweilow]

This pull request tries to implement the fixes discussed in #166

There are not yet any tests, as I'd like to get feedback on whether this is the route to go 👍

A thing that is concerning is that it would be nice to have useful errors for the situation in vadimdemedes/ink-text-input#25, while an application that supports running in CI might want to just ignore setRawMode.

Todo

• Should a helpful error be thrown if setRawMode isn't supported, with a configuration option to fall back to doing nothing?
• Write tests

[vadimdemedes]

Everything looks good to me!

Considering that you've added isRawModeSupported, I think it does make sense to throw an error if setRawMode is called, but raw mode isn't supported.

[eweilow]

Yeah, I agree.

handleSetRawMode now throws if isRawModeSupported(stdin) returns false. I've added a check for if stdin === process.stdin, to more nicely give feedback for apps not using a custom stdin. Both errors provide a link to https://github.com/vadimdemedes/ink/#israwmodesupported (which will be valid if and once this PR is merged)!

The behaviour now is also that handleSetRawMode is ignored in componentWillUnmount and handleExit if isRawModeSupported(stdin) returns false. This is because nothing is run in handleSetRawMode if it throws, so it's not necessary to clean anything up 👍

[vadimdemedes]

Don't forget to unmark this PR as draft when you think it's ready ;)

[eweilow]

The code changes are ready, but I'm not comfortable with the tests yet (I've not been able to properly run the fixtures on Windows). Going to try and wrap it up with a Mac tomorrow afternoon :)

[vadimdemedes]

Hey @eweilow, how's it going? If you don't have the time, that's ok, just let me know ;)

[eweilow]

I think it's ready, but the tests aren't perfect (I would have liked to have done more thorough testing of App, but I can't figure out how to do it 😢 - I'm too stuck in how Jest works). The ones I've written appear to pass on my machine, so that's something!

[vadimdemedes]

It uses Ava, not Jest :) What were you struggling with in tests?

[eweilow]

Ah yes, what I meant to say is that I'm too used to Jest 👍

What I also wanted to do was something like this, but it ended up troublesome because App required to be exported (which affects the public API):

test('isRawModeSupported should return correctly', t => {
	t.true(App.prototype.isRawModeSupported({ isTTY: true }));
	t.false(App.prototype.isRawModeSupported({ isTTY: false }));
});

It also feels wrong to use App.prototype to run tests like this, which should be covered by the other tests I've written.

[vadimdemedes]

You can test that by overriding stdin that Ink app uses like you did it here - https://github.com/vadimdemedes/ink/pull/172/files#diff-809f79e6e080c3848062e9756f30a67cR261. I would also prefer if you could address #172 (comment), that way the test you've mentioned wouldn't even be needed.

[eweilow]

Sorry for the delay. I've had to focus my attention writing on my degree project report.

Regarding #172 (comment) - it is tested by the two component tests implemented right now.
Another, perhaps nicer way to implement the actual function, is to simply make it a static method on App. I've done this in 0f3a31c if you'd want to take a look

But this essentially boils down to preference, where you'll have the final say.

[vadimdemedes]

Sorry for the delay. I've had to focus my attention writing on my degree project report.

No worries, this is open source :)

Another, perhaps nicer way to implement the actual function, is to simply make it a static method on App. I've done this in 0f3a31c if you'd want to take a look

I don't think I understand why it has to be a static method. Could you revert back isRawModeSupported to this?

isRawModeSupported() {
  return this.props.stdin.isTTY;
}

[eweilow]

Sure, it's now reverted to that.

[vadimdemedes]

Looking great, sorry for late reply! I will make some tiny adjustments (mostly naming) and merge it. Thank you!

[vadimdemedes]

Great work, @eweilow, this is very useful!

[vadimdemedes]

Out in 2.2.0!

[eweilow]

Awesome 👍 Now I can remove the TTY workaround I had to implement 💃

[mAAdhaTTah]

This should probably have been considered a breaking change, as throwing here breaks the expected semantics of the function. You've introduced new, unexpected behavior where it worked previously. I updated ink-testing-library, which has issues with this change, as well as ink-text-input, but there are likely other components that will now throw in places where they wouldn't previously. Doing nothing would be "nicer"–not sure what the justification for throwing > silently ignoring is.

[eweilow]

Calling setRawMode in StdinContext.Consumer only throws if the underlying stdin doesn't have stdin.isTTY truthy (which implies that stdin.setRawMode is defined). If stdin.isTTY is falsy, then calling setRawMode in the context would throw regardless since stdin.setRawMode would be undefined. It's much nicer to throw a useful error in this case, rather than just let a non-descriptive 'undefined is not a function' slip through.

My reasoning for not silently ignoring is that a component might want to not provide input at all if running in something like a CI environment (while still being able to use Ink). Ignoring a call to setRawMode might lead to a weird place where components think it's enabled, while it's not.

In your PR vadimdemedes/ink-testing-library#7 I see that you updated the incomplete mock of https://nodejs.org/api/tty.html#tty_class_tty_readstream (which before your PR excludes the isTTY property). For a normal application (that passes a real tty.ReadStream to stdin), the changes won't throw 🙂

[mAAdhaTTah]

Do I understand correctly: besides ink-testing-library, every time this function would throw with our custom check, it already does because setRawMode === undefined?

[eweilow]

That's the idea. If stream.isTTY is true, then stream.setRawMode is a function (at least that's what the Node docs say). Otherwise setRawMode === undefined and the function on the context throws just like it did before.

The function after this PR just does slightly stricter checking for when to throw and throws a nicer error.

[vadimdemedes]

I agree with @eweilow, as this was technically a bug in Ink. Ink shouldn't have allowed enabling raw mode where it's unsupported, because it will crash the process.