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

Asyncified validate + submit flow #711

Merged
merged 12 commits into from
May 6, 2020
Merged

Asyncified validate + submit flow #711

merged 12 commits into from
May 6, 2020

Conversation

radekmie
Copy link
Contributor

@radekmie radekmie commented Apr 22, 2020

Let's start with a short flashback from #644:

[...] Well, the first bridge ever was the SimpleSchema (the Meteor one), and its validator throws an error instead of returning it. It was perfectly fine, up to the point when we wanted to add onSubmitSuccess/onSubmitFailure and async validation. Of course, we managed to do so, but the solution is not very elegant.

This not very elegant solution got implemented in #51. Why it's not elegant? Because it's redundant. There's no need for such hooks if one can simply add it directly to the onSubmit. Another problem (non-elegancy) is the asynchronous validation - invented in #17 and implemented in 4a56596. It's weird and is using callbacks.

This PR introduces a new, clean flow (including #711 (comment)):

  • Bridge validators can be asynchronous.
  • Bridge validators have to return errors instead of throwing them. More background is in Validation Errors should not rely on throw/catch #644.
  • The onValidate is now (model, error?) => error? | Promise<error?>. This is far better and obvious than the callback version before.
  • Removed onSubmitSuccess and onSubmitFailure. I'm pretty sure there's no viable case for them at all. Migration is straightforward:
    -onSubmit={onSubmit}
    -onSubmitSuccess={onSubmitSuccess}
    -onSubmitFailure={onSubmitFailure}
    +onSubmit={model => {
    +  const result = onSubmit(model);
    +  result.then(onSubmitSuccess, onSubmitFailure);
    +  return result;
    +}}`
  • Separated submitting and validating states. Now validating is true only while the bridge validator or onValidate are currently running. Same for submitting - it's try only while onSubmit is running.

TODO:

  • Perform some manual tests to check if there's no visible difference with this change. It might be the case as few things got asyncified.
  • Run benchmarks to ensure no performance degradation.
  • Add missing tests.
  • Update CHANGELOG.md.

Closes #644.

@radekmie radekmie added the Type: Feature New features and feature requests label Apr 22, 2020
@radekmie radekmie added this to the v3 milestone Apr 22, 2020
@radekmie radekmie requested review from kestarumper and Monteth April 22, 2020 14:53
@codecov
Copy link

codecov bot commented Apr 22, 2020

Codecov Report

Merging #711 into v3 will increase coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##               v3     #711      +/-   ##
==========================================
+ Coverage   97.96%   98.02%   +0.05%     
==========================================
  Files         162      162              
  Lines        1669     1668       -1     
  Branches      604      602       -2     
==========================================
  Hits         1635     1635              
  Misses          8        8              
+ Partials       26       25       -1     
Impacted Files Coverage Δ
packages/uniforms/src/Bridge.ts 100.00% <ø> (ø)
...-bridge-simple-schema-2/src/SimpleSchema2Bridge.ts 100.00% <100.00%> (ø)
...rms-bridge-simple-schema/src/SimpleSchemaBridge.ts 100.00% <100.00%> (ø)
packages/uniforms/src/BaseForm.tsx 98.14% <100.00%> (-0.07%) ⬇️
packages/uniforms/src/ValidatedForm.tsx 100.00% <100.00%> (+1.61%) ⬆️

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 69d2b78...e045c3b. Read the comment docs.

@radekmie
Copy link
Contributor Author

During benchmarks, it turned out that making everything async has one flaw: setState happens before then. It means that we have at least one additional render. Right now I'm working on a solution similar to the one in BaseForm to handle both sync and async validators, onValidate, and onSubmit in any configuration.

Another problem araised while working with rejected promises. The thing is that it's not better than throw as we still cannot type it anyhow. We won't be able to type it correctly anytime soon (I don't have an idea on how to type errors as those are created in bridges, used in forms, and then analyzed by bridges), but having such a possibility would be nice. I'll experiment with actually returning errors instead, as discussed in #644.

@radekmie radekmie marked this pull request as ready for review April 27, 2020 17:21
packages/uniforms/__tests__/ValidatedForm.tsx Outdated Show resolved Hide resolved
packages/uniforms/src/ValidatedForm.tsx Show resolved Hide resolved
@radekmie radekmie merged commit 716f0bd into v3 May 6, 2020
@radekmie radekmie deleted the v3-asyncified-validation branch May 6, 2020 10:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature New features and feature requests
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants