-
-
Notifications
You must be signed in to change notification settings - Fork 587
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use stdin readable
event instead of data
#616
Use stdin readable
event instead of data
#616
Conversation
21a8cb6
to
df02408
Compare
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. |
93111eb
to
a7eacc6
Compare
a7eacc6
to
d4a564e
Compare
The current approach works but I'm not a huge fan of mixing The Node docs recommend against using this mixed approach:
I agree with this, it's not obvious that @vadimdemedes what do you think? Shall I go ahead with this approach? |
Not sure I follow. I thought this PR stop relying on |
Yes the problem is that because we need to subscribe to incoming data in 2 places ( EDIT: the alternative is to merge this as is, but it would mix both |
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. |
9c718b8 @vadimdemedes this commit does what I wanted to do. I've used the |
a0f959c
to
9c718b8
Compare
Makes sense. Thanks! |
Hi, I think this PR breaks the testkit with testing input - #627 |
By doing testing of the Shopify CLI on Windows we found that
stdin
could get in a broken (paused) state ifpause
andresume
were called in rapid succession by mounting and unmounting components that useuseInput
. 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
andresume
and switching to thereadable
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.