SBoudrias / Inquirer.js

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

rework prompt lifecycle #1536

Closed mshima closed 2 months ago

mshima commented 2 months ago

It's difficult to understand when cleanup hooks are called. I've added signal, abort and onFinally support to CancelablePromise. So the lifecycle is managed by CancelablePromise instead of cleanup callbacks.

codecov[bot] commented 2 months ago

Codecov Report

Attention: Patch coverage is 98.07692% with 1 line in your changes missing coverage. Please review.

Please upload report for BASE (main@0c03959). Learn more about missing BASE report.

Files with missing lines Patch % Lines
packages/core/src/lib/create-prompt.mts 96.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1536 +/- ## ======================================= Coverage ? 97.89% ======================================= Files ? 38 Lines ? 2380 Branches ? 631 ======================================= Hits ? 2330 Misses ? 44 Partials ? 6 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

SBoudrias commented 2 months ago

In absolute, I'd prefer not adding new APIs on top of Promise. That's also kind of hard to understand for folks new to the codebase since it's not a Node built-in. I added withResolver as-is since it's polyfill for Node 18.

Instead of exposing onFinally, could we keep the logic more similar to how it is, but instead of calling onExit, we could clear the cleanups on promise.finally? (There's a risk approaching it this way to modify the order of operations; I'm not sure without trying if it'll cause side-effects)

mshima commented 2 months ago

In absolute, I'd prefer not adding new APIs on top of Promise. That's also kind of hard to understand for folks new to the codebase since it's not a Node built-in. I added withResolver as-is since it's polyfill for Node 18.

Since a custom CancelablePromise was implemented a custom withResolver looks ok to me.

Instead of exposing onFinally, could we keep the logic more similar to how it is, but instead of calling onExit, we could clear the cleanups on promise.finally? (There's a risk approaching it this way to modify the order of operations; I'm not sure without trying if it'll cause side-effects)

I’ve tried this approach but a finally() will return a bare Promise instead of a CancelablePromise.

SBoudrias commented 2 months ago

The difference with the Promise is that it's a polyfill https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/withResolvers

SBoudrias commented 2 months ago

Made 2 demos of alternative to that problem #1542 and #1543 - they're not ideal, so I'd love your review and your take if you have any other ideas to improve this!