search

SBoudrias / Inquirer.js

A collection of common interactive command line user interfaces.
MIT License
19.67k stars 1.28k forks source link

`@inquirer/select`: prompt closes after timeout on `CTRL+C` or `CTRL+D` #1454

Open matteosacchetto opened 6 days ago

matteosacchetto commented 6 days ago

When terminating the @inquirer/select prompt with CTRL+C or CTRL+D it properly throws the Error User force closed the prompt with 0 null Though, it does so after the timeout searchTimeoutRef is expired.

Example reproduction In this reproduction, try running node index.mjs and stop it with CTRL+C or CTRL+D. To compare the behavior, I also added the index2.mjs which uses @inquirer/input

By debugging the behavior, I noticed that on CTRL+C or CTRL+D, the useKeypress callback is called, the timeout is cleared and then, since the key combination does not match any of the if/else if statements, it enters the else condition. Here the timeout is created again, and after that timeout expires the Error is thrown.

https://github.com/SBoudrias/Inquirer.js/blob/ac687691f6847816e89a80a589e82738bc4c13b0/packages/select/src/index.mts#L124-L142

To address it, the simplest solution would be to change the else statement to an else if checking that it does not match CTRL+C or CTRL+D. Though, other strategies may be used.

Once we agree on a strategy, I am willing to open a PR to address this.

Tested version: @inquirer/select: 2.3.8

SBoudrias commented 6 days ago

Interesting... I think a more universal solution would be to modify useKeypress inside the core itself to ignore CTRL+C and CTRL+D (and macOS/linux equivalent.) Otherwise that'll be a weird edge cases for any prompts forgetting to check for that code.

I wonder if you'd encounter the same issue with a prompt with an async validator? (where there's a timeout to animate the loading animation.)

matteosacchetto commented 5 days ago

While that can help, I do not think it fully addresses the problem

The reason being that, if the timeout is started and then CTRL+C or D is clicked, it still waits for the whole timeout before closing the prompt and throwing the error even if the useKeypress is not triggered by that key combination.

I think that a solution could be, on top of ignoring those key combinations, to also provide some kind of hook/callback which gets called when the prompt exit condition is triggered. This would allow for resource cleanup, like deleting the timeout.

SBoudrias commented 5 days ago

Ah you're right!

I think as of now cleanups from useEffect will trigger on force exit šŸ¤” - e.g useEffect(() => () => cleanup(), []);. I'm not 100% sure, so we'd need to validate this (and if not, I'm pretty sure it should!)

I think we can keep this as-is in the core, and only use this inside select. WDYT?

matteosacchetto commented 2 days ago

I tried with the useEffect method, and I can confirm that it gets called on exit. Though it seems that the exit handler only gets called after the timeout is expired https://github.com/SBoudrias/Inquirer.js/blob/85a3d400014280c5cdae03f6de63d2256ea6b2df/packages/core/src/lib/create-prompt.mts#L38-L43

I am still investigating the behavior to understand why it's behaving in this way