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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

initial useKeypress #147

Closed
wants to merge 1 commit into from
Closed

Conversation

jedahan
Copy link

@jedahan jedahan commented Mar 8, 2019

first draft of #138

todo

  • fix typescript definition (never written typescript before)
  • run npm test
    - [ ] write documentation
  • write example
  • write some tests

@sindresorhus
Copy link
Collaborator

This needs to handle the case if raw mode is already enabled when this hook is used, it should not be disabled when this hook ends.

@vadimdemedes
Copy link
Owner

That's actually going to be handled by setRawMode() itself, so hook doesn't need to re-implement that logic :)

src/hooks/useKeypress.js Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
src/hooks/useKeypress.js Outdated Show resolved Hide resolved
test/keypress.js Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
@jedahan
Copy link
Author

jedahan commented Mar 11, 2019

So I got stuck on my test - it would be cool if ava could allow you to get a handle to stdin/stdout or the subprocess when running tests so we can simulate keypresses etc.

@vadimdemedes
Copy link
Owner

See https://yarnpkg.com/en/package/node-pty on how you can write to stdin of the process created by node-pty (which is used in test/exit.js) ;)

@vadimdemedes
Copy link
Owner

Hey @jedahan, do you have time to finish this PR? Totally ok if not ;)

@jedahan
Copy link
Author

jedahan commented May 27, 2019

Unfortunately I cannot get master to npm install on my laptop now. Which is weird because a fresh checkout of node-pty 0.8.1 does npm install correctly, but thats where its erroring

@vadimdemedes
Copy link
Owner

Try using Node.js v10.x.

@sindresorhus
Copy link
Collaborator

@jedahan Did you manage to get it to npm install?

@jedahan
Copy link
Author

jedahan commented Jun 20, 2019

npx -p node@10 npm install && npx -p node@10 npm test passes

@jedahan
Copy link
Author

jedahan commented Jun 20, 2019

Working on fixing the test now

@jedahan jedahan marked this pull request as ready for review June 21, 2019 00:24
examples/usekeypress/usekeypress.js Show resolved Hide resolved
examples/usekeypress/usekeypress.js Outdated Show resolved Hide resolved
examples/usekeypress/usekeypress.js Outdated Show resolved Hide resolved
examples/usekeypress/usekeypress.js Outdated Show resolved Hide resolved
examples/usekeypress/usekeypress.js Outdated Show resolved Hide resolved
index.d.ts Show resolved Hide resolved
index.d.ts Show resolved Hide resolved
index.d.ts Show resolved Hide resolved
test/fixtures/handles-keypresses.js Outdated Show resolved Hide resolved
test/fixtures/handles-keypresses.js Show resolved Hide resolved
@jedahan
Copy link
Author

jedahan commented Sep 7, 2019

I'm rebasing

@jedahan
Copy link
Author

jedahan commented Sep 7, 2019

I think I've made all the requested changes

@vadimdemedes
Copy link
Owner

Hey @jedahan, I've tried running node examples/usekeypress and process exits right away, after rendering the output. It doesn't wait for user input.

@vadimdemedes
Copy link
Owner

vadimdemedes commented Sep 15, 2019

I digged deeper and was able to fix it by replacing useEffect with useLayoutEffect. The problem with useEffect is that it fires the handler after rendering is finished, so setRawMode isn't called fast enough. Process sees that there are no more tasks left and exits.

I also decided to switch from using keypress to data event, because when user pastes text into terminal, multiple keypress events are emitted for each character. This is not as convenient as using data event, which contains the whole string in one event.

I'll open another PR, which includes your changes and fixes these things. I will make sure to mention you, of course. Thank you for all your work!

@vadimdemedes
Copy link
Owner

Opened #227 to take this PR further. I would've asked @jedahan, but I want to release it asap.

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

Successfully merging this pull request may close these issues.

4 participants