Ink 4.4.0 doesn't wait for user input from `stdin` on second mount
threepointone opened this issue · 15 comments
Hiya! Looks like this commit #616 broke something with input handling in 4.4.0. I managed to make a reproduction here https://github.com/threepointone/ink-repro. By running it, you'll see that hitting enter once will log the next message, but quit immediately after (with exit code 0).
With 4.3.1, it'll wait for 3 presses, as expected.
Thanks for the exemplary bug report, Sunil!
I traced the issue down to https://github.com/vadimdemedes/ink/pull/616/files#diff-86963609bec6fda3d34233676d5ae33f6aa4b9bbe2758a10cd4863b704b37093R178, which gets called during unmount(). The way I understand it, is it basically tells Node.js "I don't need data from process.stdin anymore, feel free to quit if there's no I/O happening".
I verified this by adding a process.stdin.ref() (the opposite of unref()) after the first waitForPress() in your reproduction → https://github.com/threepointone/ink-repro/blob/main/src/index.tsx#L27.
@matteodepalo Do we have to unref() the process.stdin in Ink? Can we remove it?
@matteodepalo Just saw your PR (#622), thanks for the fix! Could you elaborate why we need to unref() in the first place? I'm wondering if it's going to have other side effects, in case other code outside of Ink depends on process.stdin data.
@threepointone Just published a 4.4.1 with @matteodepalo's fix while we're figuring this stuff out.
Amazing thank you! I’ll try this later when I’m at my laptop. Thanks for the quick turnaround!
4.4.1 appears to have fixed my specific issue, thank you!
Looks like it broke ink-testing-library tests for components that use useInput, I put up a PR with a fix vadimdemedes/ink-testing-library#23
Could you elaborate why we need to unref() in the first place? I'm wondering if it's going to have other side effects, in case other code outside of Ink depends on process.stdin data.
Basically, without unref() all the useInput tests would timeout because the node process wasn't allowed to exit. This is because adding any event listener will keep the process open, even if you call .off. This wasn't just in tests, our CLI commands wouldn't end "naturally" after everything was processed.
Before we had pause which would accomplish the same goal as unref, but had other issues, especially in Windows, and was outdated.
Thank you so much for merging!
I think #616 has actually been a breaking change and should've been released in a major version bump. It looks like it broke user-land code in several places, which shouldn't happen in a minor version bump (this issue, vadimdemedes/ink-testing-library#23, #627).
The best course of action seems to be to release a patch release with reverted readable change in #616 and then release a major version with that code included again.
What do you think?
To me that change isn't breaking code that uses the ink library itself because it changes the way an internal event is managed. The breaking change in ink-testing-library is because the Stdin class emits a data event manually, but I don't think that is part of the public API. If you stick to what is documented in the readme then you shouldn't get any breaking changes.
If you don't think that's enough then yes I agree that we should revert the change in a patch and ship the readable work as part of version 5.
Although I was thinking that useStdin users might see this as a breaking change, since stdin would emit data events.
Yeah I understand this. It mostly depends on what you consider to be public and private APIs. data events are not documented so I assumed it could be treated as private.
Also, according to the docs the data event will still be emitted when we call stream.read(), so anyone subscribing to that event won't have any breaking change. The problem in ink-testing-library was the other way around, we needed to trigger the useInput callback and the only way to do that is to emit the readable event.
Should I send a PR to ink-testing-lbrary that brings back emit('data',...?
It broke our tests when we tried to upgrade (we use the testing library for our low level components).
I went now and manually tested one of our product flows, user input works fine AFAICT.
Any updates on this - anytime I try to use an Ink component that requires user input the app immediately closes