feat: add async validation - #583
Conversation
This adds the ability to have async validation to all prompts. **For now, only the text prompt will render a validating message.** Every other prompt needs its own individual follow-up PR to add support for rendering the new `validating` state. They will all accept an async validator but will not show a message while validating right now.
🦋 Changeset detectedLatest commit: ad9b382 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
commit: |
dreyfus92
left a comment
dreyfus92
left a comment
There was a problem hiding this comment.
awesome job james, left a few comments 👀
| } | ||
|
|
||
| private onKeypress(char: string | undefined, key: Key) { | ||
| private async onKeypress(char: string | undefined, key: Key) { |
There was a problem hiding this comment.
this is now async but the emitter doesn't await it, so input keeps flowing while a validation is in flight. so i feel like there are 2 problems maybe i'm overthinking u tell me:
- press return twice fast and you get 2 concurrent validations. the first one can resolve, emit
submit, close the prompt and then second resolves and tries to render/emit on a finalizad prompt. state gets tangled. - the user can keep typing while validating, mutating
valuemid-flight. then the error or submit applies to a stale value, and the rendered dimmed input won't match what was validated.
There was a problem hiding this comment.
good catch!
we may be able to just return early when we're validating, i.e. ignore all key presses during validation
| ) { | ||
| this.state = 'validating'; | ||
| this.render(); | ||
| problem = await problemResult; |
There was a problem hiding this comment.
if the async validator throws (network error, w/e), this will throw inside an un-awaited async handler. that's an unhandled promise rejection, which kills the process in node. probably it might be better to do a try/catch? so that routes the thrown error into this.error / state = 'error' instead. also maybe would be worth a test case too.
| problemResult !== undefined && | ||
| typeof problemResult !== 'string' && | ||
| !(problemResult instanceof Error) |
There was a problem hiding this comment.
| problemResult !== undefined && | |
| typeof problemResult !== 'string' && | |
| !(problemResult instanceof Error) | |
| problemResult instanceof Promise |
wouldn't be a more clean approach for the check to be set this way?
3 participants
This adds the ability to have async validation to all prompts.
For now, only the text prompt will render a validating message.
Every other prompt needs its own individual follow-up PR to add support for rendering the new
validatingstate. They will all accept an async validator but will not show a message while validating right now.Closes #92
Type of change
Checklist
pnpm testpasses (or targeted tests for my change)pnpm formathas been runAI-generated code disclosure