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

feat(inquirer): add AbortSignal support #1524

Merged
merged 1 commit into from
Sep 7, 2024

Conversation

mshima
Copy link
Contributor

@mshima mshima commented Aug 28, 2024

Fixes #1521

Copy link

codecov bot commented Aug 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (main@80c6c57). Learn more about missing BASE report.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1524   +/-   ##
=======================================
  Coverage        ?   98.25%           
=======================================
  Files           ?       37           
  Lines           ?     2403           
  Branches        ?      653           
=======================================
  Hits            ?     2361           
  Misses          ?       36           
  Partials        ?        6           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mshima mshima changed the title add AbortSignal support feat: add AbortSignal support Aug 28, 2024
packages/prompts/README.md Outdated Show resolved Hide resolved
packages/inquirer/inquirer.test.mts Outdated Show resolved Hide resolved
@SBoudrias
Copy link
Owner

I took the liberty to:

  1. Add more documentation and update the timeout demo. AbortSignal is a much better interface, so I want to make sure we surface it :)
  2. Refactored the cleanup logic in @inquirer/core - it wasn't great, and I felt it caused some struggle. It'd have been simpler without promise.cancel() but oh well, hindsight is 20/20! I'm looking forward to Promise.withResolver to make it to all LTS to simplify that further.

I'm very happy with the state of that feature inside @inquirer/core. If you want to merge this sooner, we could extract while we sidebar the inquirer interface question.

@mshima mshima force-pushed the abort-signal branch 2 times, most recently from 855278c to 0140d6f Compare September 2, 2024 22:50
@mshima
Copy link
Contributor Author

mshima commented Sep 2, 2024

AbortSignal.any() requires node 18.17.0 and 20.3.0.
I don't know a easy way to replace it.

@mshima
Copy link
Contributor Author

mshima commented Sep 3, 2024

This PR now adds Signal support to the createPromptModule, to inquirer.prompt() and uses signal to handle promise.ui.close().

@SBoudrias
Copy link
Owner

Did you look into polyfilling AborySignal.any() yet?

@mshima
Copy link
Contributor Author

mshima commented Sep 3, 2024

Did you look into polyfilling AborySignal.any() yet?

There is AbortSignal polyfills but not any().

@SBoudrias SBoudrias changed the title feat: add AbortSignal support feat(inquirer): add AbortSignal support Sep 7, 2024
@SBoudrias SBoudrias merged commit b68860b into SBoudrias:main Sep 7, 2024
9 of 10 checks passed
@SBoudrias
Copy link
Owner

Great! Thanks for all the back and forth on this one 🤞🏻

@mshima mshima deleted the abort-signal branch September 7, 2024 20:10
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.

Add AbortSignal to @inquirer/core.
2 participants