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

feat: async validation support #61

Closed
wants to merge 11 commits into from

Conversation

jopmiddelkamp
Copy link

Description

I've added code to support async form validations.

Type of Change

  • ✨ New feature (non-breaking change which adds functionality)
  • 🛠️ Bug fix (non-breaking change which fixes an issue)
  • ❌ Breaking change (fix or feature that would cause existing functionality to change)
  • 🧹 Code refactor
  • ✅ Build configuration change
  • 📝 Documentation
  • 🗑️ Chore

@jopmiddelkamp
Copy link
Author

@felangel @jorgecoca could one of you take a look at this PR?

@jopmiddelkamp
Copy link
Author

@felangel @jorgecoca

@felangel
Copy link
Contributor

felangel commented Oct 6, 2022

@felangel @jorgecoca

Sorry for the delay! I’ll have a look shortly 👍

@jopmiddelkamp
Copy link
Author

jopmiddelkamp commented Oct 27, 2022

@felangel @jorgecoca @renancaraujo @wolfenrain

Sorry for the delay! I’ll have a look shortly 👍

Friendly reminder :)

@jopmiddelkamp
Copy link
Author

jopmiddelkamp commented Nov 10, 2022

Another friendly reminder :) @felangel @jorgecoca @renancaraujo @wolfenrain

Would really appreciate it if one of you guys could free up some time to look at this PR and let me know if you agree or reject the change. Then I can focus again on getting the ugly temporary package out of my project..

@wolfenrain
Copy link
Member

Hi @jopmiddelkamp, sorry for the delay we will try to get to it this week!

For now i'll at least run the workflows to validate if it also succeeds :D

@jopmiddelkamp
Copy link
Author

Hi @jopmiddelkamp, sorry for the delay we will try to get to it this week!

For now i'll at least run the workflows to validate if it also succeeds :D

Thanks/bedankt @Wolferain!

@jopmiddelkamp jopmiddelkamp changed the title async validation support feat: async validation support Nov 17, 2022
@renancaraujo renancaraujo self-assigned this Nov 28, 2022
@renancaraujo
Copy link
Contributor

Hello @jopmiddelkamp, Thanks for all that effort, and forgive us for the huge delay in responding 🙏🏽.

After looking into the code and having a conversation with the team, we decided that async validation has use cases that may need a more careful look into. So we won't merge this.
Also, I left some more comments below with feedback on some points we noticed with your approach.

Thanks again for your contribution!

@@ -0,0 +1,6 @@
import 'package:example/async/ui/app.dart';
Copy link
Contributor

Choose a reason for hiding this comment

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

I appreciate the example app from a more "real world usage" point of view, but it defeats the purpose of the example of allowing people to understand what a package API looks like when used.

final E? error;

/// The validation status of the [AsyncFormzInput].
final AsyncFormzInputValidationStatus validationStatus;
Copy link
Contributor

Choose a reason for hiding this comment

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

With the validator being independent of the input class, there is no safeguard (and out-of-the-box clarity) on who should change this value.

The package usage becomes too much dependent on the boilerplate to work consistently.

/// ```
/// {@endtemplate}
@immutable
abstract class FormzInputBase<T, E> {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a nice abstraction from the premise that sync and async inputs would have different classes. That may not necessarily be the case. See this proposal.

@gugadev
Copy link

gugadev commented Apr 16, 2023

@renancaraujo could this PR be taken as a base for a new one? Or better raise a new PR having this proposal in mind?

@renancaraujo
Copy link
Contributor

@gugadev yeah, this is still on our backlog of things to do. The specifics of this pr (including the things exposed by comments) for sure will be base for the implementation.

@gabrielchaves7
Copy link

@renancaraujo I have made this one #93 as a initial setup to bring back to discussion if using FutureOr as mentioned by #15 (comment) is a good idea. I would appreciate more thoughts on how we can do the async validation. If we want to have FutureOr at the FormzInput abstract class we would force who is just using sync validators to use the await keyword as you can see at my initial PR.

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.

6 participants