Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

setRawMode fails when running with non-TTY stdin #166

Closed
eweilow opened this issue Mar 19, 2019 · 8 comments
Closed

setRawMode fails when running with non-TTY stdin #166

eweilow opened this issue Mar 19, 2019 · 8 comments

Comments

@eweilow
Copy link
Contributor

eweilow commented Mar 19, 2019

Before the rest of the issue, I must say that Ink is amazing 馃憤

An issue I've encounted is that when Ink is running in a process where process.stdin.setRawMode is undefined (for instance in some CI environments), these lines will throw:
https://github.com/vadimdemedes/ink/blob/master/src/components/App.js#L73
https://github.com/vadimdemedes/ink/blob/master/src/components/App.js#L83

This behaviour is detailed by the Node docs on https://nodejs.org/api/tty.html#tty_readstream_setrawmode_mode

When Node.js detects that it is being run with a text terminal ("TTY") attached, process.stdin will, by default, be initialized as an instance of tty.ReadStream and both process.stdout and process.stderr will, by default be instances of tty.WriteStream. The preferred method of determining whether Node.js is being run within a TTY context is to check that the value of the process.stdout.isTTY property is true

After a search for setRawMode, it seems that this issue has resurfaced: #40

While the easy fix would be to wrap all those calls with a check for stdin.isTTY, that might lead to unexpected behaviour when an app actually expects raw mode to have been set!

@vadimdemedes
Copy link
Owner

While the easy fix would be to wrap all those calls with a check for stdin.isTTY, that might lead to unexpected behaviour when an app actually expects raw mode to have been set!
Not sure there's other solution, since it's impossible to listen for user input in non-tty env, isn't it? I think Ink should probably do nothing when setRawMode is called in a non-tty env. What do you think?

Before the rest of the issue, I must say that Ink is amazing 馃憤

Thank you!

@eweilow
Copy link
Contributor Author

eweilow commented Mar 20, 2019

Not sure there's other solution, since it's impossible to listen for user input in non-tty env, isn't it? I think Ink should probably do nothing when setRawMode is called in a non-tty env. What do you think?

What might be good, since it's always nice to have a proper fallback, is to have some way for the app rendered by Ink to determine if setRawMode is available! I believe this might be useful for features like #147

In most cases, Ink would just ignore that it can't enable raw mode, but for some cases an app might want to entirely disable an input component. So just doing nothing when setRawMode is called in a non-tty environment works, but there could be additional functionality added to handle it nicely as well!

@mAAdhaTTah
Copy link
Contributor

Yeah, I actually ran into this. I am trying to migrate some tests, where I would cp.spawn the cli and use the exposed stdin to write to the process and confirm the stdout matches my expectations as well as confirm the files written to the fs matched. The cli would error on the call to setRawMode and fail the tests.

I had originally attempted to use pty.js, but may have to give node-pty from the linked thread a shot. That said, it should still work w/o the call to setRawMode, so I kinda feel like the conditional would be a nice addition.

Also happy to PR this as well :).

@vadimdemedes
Copy link
Owner

What might be good, since it's always nice to have a proper fallback, is to have some way for the app rendered by Ink to determine if setRawMode is available!

@eweilow You don't need Ink for that, see how to check for a tty env: https://stackoverflow.com/questions/20585729/how-to-detect-if-nodes-process-stdout-is-being-piped.

@eweilow
Copy link
Contributor Author

eweilow commented Mar 21, 2019

@vadimdemedes What if <StdinContext> exposed an additional isRawModeSupported bool?
It might lead to more 3rd party components adopting a nicer fallback if it was clear in the docs, even if it is currently as simple as checking if stdin.isTTY is true or not. Another option would be to simply add info on how to check for that in the docs of StdinContext 馃憤

@mAAdhaTTah
Copy link
Contributor

mAAdhaTTah commented Mar 21, 2019

I was able to solve my problem with node-pty, so I no longer need this. I will just note that components like ink-text-input might also need to add some handling for this if we don't solve it in Ink proper. My initial thought was to wrap setRawMode such that if it doesn't exist we just don't call it, which would keep downstream components from needing to add extra internal logic to check.

@eweilow
Copy link
Contributor Author

eweilow commented Mar 22, 2019

I've started a PR (#172), but realized that it might be useful to throw a nice error by default rather than just ignoring that setRawMode cannot be called.

Otherwise this situation would end up with being able to not input anything (which would be confusing!) vadimdemedes/ink-text-input#25

@vadimdemedes
Copy link
Owner

Nice, thanks @eweilow! Let's continue discussion there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants