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

async validate #94

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

async validate #94

wants to merge 6 commits into from

Conversation

ulken
Copy link
Collaborator

@ulken ulken commented Mar 2, 2023

Took a stab at supporting async validation.

Currently, the UX is suffering, though. The program seemingly hangs, without any feedback to the user. We should probably kick off a spinner or similar.

Potentially by emitting a validate event from core to allow styled prompts to take appropriate action? Or introducing a new state and doing an intermediate render?

Related to #92

@ulken ulken requested a review from natemoo-re March 2, 2023 23:00
@ulken ulken self-assigned this Mar 2, 2023
@changeset-bot
Copy link

changeset-bot bot commented Mar 2, 2023

🦋 Changeset detected

Latest commit: 19a1903

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@clack/prompts Minor
@clack/core Minor

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

@ulken
Copy link
Collaborator Author

ulken commented Mar 5, 2023

@natemoo-re thoughts?

@natemoo-re
Copy link
Member

This looks good! I pushed a change to set a validate state, but it only triggers after 300ms so the UI doesn't flash a temporary loading state if the validation is pretty fast.

Also adds a validate state, which can be triggered during a long async validation.

@natemoo-re
Copy link
Member

Feel free to merge if you feel comfortable with this!

@ulken
Copy link
Collaborator Author

ulken commented Mar 5, 2023

Nice solution! I'll make sure to try it out and merge if appeased 😉

examples/basic/index.ts Outdated Show resolved Hide resolved
packages/core/src/prompts/prompt.ts Outdated Show resolved Hide resolved
@ulken ulken marked this pull request as ready for review March 5, 2023 23:00
@ulken
Copy link
Collaborator Author

ulken commented Mar 5, 2023

The password prompt seems to be broken now (compared to the main branch — even after handling the validate state). When submitting the masked value is seemingly cleared.

@ulken
Copy link
Collaborator Author

ulken commented Mar 6, 2023

It seems like Promise.race() doesn't work as you expect with setTimeout(). It's not cancelled (thus always resolved and rendered). You would need to pass a signal (AbortController) to cancel the timeout.

@@ -172,7 +177,17 @@ export default class Prompt {

if (key?.name === 'return') {
if (this.opts.validate) {
const problem = this.opts.validate(this.value);
this.state = 'validate';
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the race/timeout is sorted, shouldn't this move closer to the render? No need to set it if never rendered?

@ulken
Copy link
Collaborator Author

ulken commented Mar 6, 2023

Would have to use something like: https://stackoverflow.com/a/39852372

@ulken
Copy link
Collaborator Author

ulken commented Mar 6, 2023

Gah, for unknown reason, the password prompt doesn't like onKeypress to be async.

Simply add an await Promise.resolve() in there and the input is cleared when pressing Enter.

Not sure if readline doesn't like async or something.

@ulken
Copy link
Collaborator Author

ulken commented Mar 8, 2023

Pushed fix for the timeout issue, but have yet to resolve the password prompt not working. I can't understand why it works for text but not password...

@Mist3rBru
Copy link
Contributor

@natemoo-re @ulken @cpreston321 I looked at this, and seems the password prompt was losing the prompt context during async validation, I fixed it on this commit d258379, please check it out!

@negativems
Copy link

How is the progress on this?

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