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

Promise variants of form.validate() and field.validate() #218

Merged

Conversation

voxpelli
Copy link
Contributor

@voxpelli voxpelli commented Jan 19, 2021

Fixes #217

Also:

  • async module is removed and replaced with a tiny tiny helper function
  • field.validate() now always returns an error, previously it only did so on required errors
  • due to the fact that field.validate() rarely returned an error, the functionality of validatePastFirstError was pretty much never respected and the tests was also set up to expect validatePastFirstError to not function. So in reality validatePastFirstError has been active for most cases for most users, so the restoration of it's functionality can almost be a bit breaking 🤔 Any thoughts @ljharb?

Are the tests enough for form.validate()? Its getting late here so I'm running out of time a bit, so I'm opening this PR now anyways so that this code won't be stuck on my computer

To do / decide:

  • How to handle a now functional validatePastFirstError
  • Update README with these promise variants of .validate() and the one from Add a promised based handle() #215
  • Once released, do the same, but for the types at DefinitelyTyped, or bring the types in here? (Bringing them here would be easier as then they can be updated prior to release)

@codecov-io
Copy link

codecov-io commented Jan 19, 2021

Codecov Report

Merging #218 (7a26a2d) into master (86c929d) will increase coverage by 0.99%.
The diff coverage is 88.09%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #218      +/-   ##
==========================================
+ Coverage   56.04%   57.04%   +0.99%     
==========================================
  Files          12       13       +1     
  Lines        1092     1129      +37     
  Branches      268      279      +11     
==========================================
+ Hits          612      644      +32     
- Misses        480      485       +5     
Impacted Files Coverage Δ
lib/fields.js 97.12% <78.57%> (-2.11%) ⬇️
lib/forms.js 88.65% <81.81%> (-1.00%) ⬇️
lib/async.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 86c929d...7a26a2d. Read the comment docs.

@ljharb ljharb force-pushed the feature/optional-promise-based-validator branch from d4d1dd3 to 7a26a2d Compare January 20, 2021 00:30
@ljharb
Copy link
Collaborator

ljharb commented Jan 20, 2021

I rebased this. I'd like to remove async in its own PR, and tests fail after the first commit, and that's concerning.

+ validatePastFirstError is respected
+ field.validate() now always returns an error and that is tested
@voxpelli voxpelli force-pushed the feature/optional-promise-based-validator branch from 7a26a2d to 7322f7a Compare March 13, 2024 18:39
@voxpelli
Copy link
Contributor Author

Rebased it and it passes now

@ljharb ljharb force-pushed the feature/optional-promise-based-validator branch 2 times, most recently from fbe36fb to 066f70a Compare March 15, 2024 23:20
@ljharb
Copy link
Collaborator

ljharb commented Mar 15, 2024

It was failing between the two commits, but i tweaked the test plan count and it seems good now.

@ljharb ljharb merged commit 066f70a into caolan:master Mar 16, 2024
317 checks passed
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 a Promise API side-by-side with the callback API of .validate()
3 participants