Skip to content

fix(manager): allow multiple field listener validators #758

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Aug 31, 2020
Merged

fix(manager): allow multiple field listener validators #758

merged 5 commits into from
Aug 31, 2020

Conversation

Hyperkid123
Copy link
Member

@Hyperkid123 Hyperkid123 commented Aug 20, 2020 •
edited
Loading

part of #739

I have added composeValidators but then I have realized, it's not usable for this feature. I have not added test but we will need this function later anyway.

@Hyperkid123 Hyperkid123 added the State manager Form state manager packages. Will be used a state management package for the form renderer. label Aug 20, 2020
@Hyperkid123 Hyperkid123 requested a review from rvsia August 20, 2020 12:51
@Hyperkid123 Hyperkid123 mentioned this pull request Aug 20, 2020
Closed
rvsia
rvsia reviewed Aug 20, 2020
View reviewed changes
Copy link
Contributor

@rvsia rvsia left a comment

Choose a reason for hiding this comment

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

Can you please add test cases?

registerField({name: 'field', validate: fn1})
registerField({name: 'field', validate: fn2})
  1. two sync validations - pass & fail
  2. two async validations - pass & fail
  3. combine sync and async - pass & fail

@codecov
Copy link

codecov bot commented Aug 20, 2020 •
edited
Loading

Codecov Report

Merging #758 into state-manager will increase coverage by 0.00%.
The diff coverage is 96.96%.

Impacted file tree graph

@@              Coverage Diff               @@
##           state-manager     #758   +/-   ##
==============================================
  Coverage          93.15%   93.16%           
==============================================
  Files                222      223    +1     
  Lines               3784     3804   +20     
  Branches            1182     1188    +6     
==============================================
+ Hits                3525     3544   +19     
- Misses               259      260    +1
Impacted Files Coverage Δ
...form-state-manager/src/files/compose-validators.ts 95.00% <95.00%> (ø)
...ckages/form-state-manager/src/utils/manager-api.ts 95.58% <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 6cdad9f...2aac294. Read the comment docs.

@Hyperkid123 Hyperkid123 force-pushed the manager-field-configs branch from cebf2ba to f4578c8 Compare August 21, 2020 07:26
@Hyperkid123
fix(manager): rebuild validation engine
62aba51 
Validation can now correctly evaluate fields with multiple registered validators.
Every validator is converted to promise, regardless if it is async or
sync validator. This allows us to properly evaluate fields with multiple
async validators. Sync validation order is preserved due to the
sequential nature of the code execution and immediate promise
resulution.

One downside is that we have run tests with setImmediate to wait for the
manager api state update.

Benefit is, that we can have more than one async validator and can it
does not have to be at the start of the validate array as it is right
now in the renderer.
@Hyperkid123 Hyperkid123 force-pushed the manager-field-configs branch from f4578c8 to 62aba51 Compare August 21, 2020 07:39
@Hyperkid123
Copy link
Member Author

@rvsia well turns out I had to re-do the whole validation process because there was an issue with concurrency. But we should have even more powerful validation support right now. We can have unlimited number of async validators with the all-new composeValidators function.

@rvsia
Copy link
Contributor

rvsia commented Aug 31, 2020

@Hyperkid123

One downside is that we have run tests with setImmediate to wait for the manager api state update.

Won't be this too huge breaking change for all applications using DDF?

@Hyperkid123
Copy link
Member Author

@rvsia yeah it will. What we can do, is first run all the validators, check if there is an async validation and return a promise based on that.

That is an option we probably should and could do. Async validation test must have async-await/setImmediate/timeout anyway.

@rvsia
Copy link
Contributor

rvsia commented Aug 31, 2020

@Hyperkid123

What we can do, is first run all the validators, check if there is an async validation and return a promise based on that.

Yeah, sounds good. Run them and just compose async validators. Do you want to do it in this PR?

@Hyperkid123
Copy link
Member Author

Hyperkid123 commented Aug 31, 2020 •
edited
Loading

Yes I will. In this PR

@Hyperkid123
Copy link
Member Author

@rvsia the compose validators will only return a promise if one or more validators are asynchronous.

@Hyperkid123 Hyperkid123 force-pushed the manager-field-configs branch from c3da8f3 to 2aac294 Compare August 31, 2020 12:41
@rvsia rvsia merged commit a6ec319 into state-manager Aug 31, 2020
@Hyperkid123 Hyperkid123 deleted the manager-field-configs branch August 31, 2020 13:11
@Hyperkid123
Copy link
Member Author

🎉 This PR is included in version 3.1.0 🎉

The release is available on

Demo can be found here!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rvsia rvsia rvsia approved these changes

Assignees
No one assigned
Labels
released State manager Form state manager packages. Will be used a state management package for the form renderer.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Hyperkid123 @rvsia