Use stdin readable event instead of data

[matteodepalo]

By doing testing of the Shopify CLI on Windows we found that stdin could get in a broken (paused) state if pause and resume were called in rapid succession by mounting and unmounting components that use useInput. Not all components would do this which leads me to believe it's a timing issue.

Here's a video showcasing one of our text prompts getting into this broken state.

04-30-dwnm8-36awe.mp4

I've tracked down the issue and it seems like removing pause and resume and switching to the readable event fixes things. I don't think this project needs to keep supporting node <10 right? If so, this change should create no backwards compatibility issues.

[vadimdemedes]

Thank you, great catch! You're right, Ink doesn't need to support old Node.js versions before 14.x.

I pinned the XO version in 80239ca, so could you undo the lint fixes you've applied in this PR? They shouldn't be needed anymore.

[matteodepalo]

The current approach works but I'm not a huge fan of mixing readable and data. The reason I have to do it is that calling read() consumes the data, so components subscribed to readable won't be receiving anything because the App event has already consumed all data in stdin. Calling read() triggers a data event so sub-components can received data that way.

The Node docs recommend against using this mixed approach:

The Readable stream API evolved across multiple Node.js versions and provides multiple methods of consuming stream data. In general, developers should choose one of the methods of consuming data and should never use multiple methods to consume data from a single stream. Specifically, using a combination of on('data'), on('readable'), pipe(), or async iterators could lead to unintuitive behavior.

I agree with this, it's not obvious that data is there to catch data after read() has been called.
I think the best solution is to create a custom EventEmitter, put it in the StdinContext and emit custom events that components using useInput can subscribe to. This could also allow us to emit an event like ready that tests can subscribe to so that they can wait before sending data, instead of waiting for 1000ms in certain places.

@vadimdemedes what do you think? Shall I go ahead with this approach?

[vadimdemedes]

Not sure I follow. I thought this PR stop relying on data event and starts using readable exclusively?

[matteodepalo]

Yes the problem is that because we need to subscribe to incoming data in 2 places (App component and useInput) we cannot use readable for both since once the data is consumed by one event then the other event won't see anything. data was working fine because it was triggering in both places. That's why I proposed to create a new event emitter that useInput can subscribe to instead of subscribing directly to stdin's readable event.

EDIT: the alternative is to merge this as is, but it would mix both data and readable. This works without using pause and resume, the stream is remains in paused state because a readable event is attached before the data one. I don't quite like this hidden order of events changing the behavior of the stream.

[matteodepalo]

I'll write the code I have in mind and ask for feedback there, probably easier to review 😄 I was just a bit wary of adding a new exported attribute in useStdin.

[matteodepalo]

9c718b8 @vadimdemedes this commit does what I wanted to do. I've used the internal_ naming convention because it's not going to be used directly by consumers.

[vadimdemedes]

Makes sense. Thanks!

[ohana54]

Hi, I think this PR breaks the testkit with testing input - #627