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

Upgrade reactive_forms to ^15.0.0 #87

Closed
ChopinDavid opened this issue May 28, 2023 · 8 comments · Fixed by artflutter/reactive_forms_widgets#102
Closed

Upgrade reactive_forms to ^15.0.0 #87

ChopinDavid opened this issue May 28, 2023 · 8 comments · Fixed by artflutter/reactive_forms_widgets#102

Comments

@ChopinDavid
Copy link
Contributor

ChopinDavid commented May 28, 2023

ReactiveForms was recently updated to allow FormControl<dynamic> to be passed as a control to fb.group. This would allow us to register dynamic FormControls in lib/ui/survey_element_factory.dart, which we should be able to do given that many elements can represent either a String or a num.

Upgrading this dependency would allow me to resolve #71 because I have written some tests that ensure RadioGroup can handle being provided either an int or a String.

@ChopinDavid
Copy link
Contributor Author

I went ahead and created this PR in an effort to resolve this issue without having to use dependency_overrides:. Will be able to resolve this issue once that PR is merged...

@laogao
Copy link
Contributor

laogao commented May 29, 2023

reactive_forms ^15.0.0 introduced a breaking change (among others): validators are now of type List<Validator<dynamic>> instead of List<ValidatorFunction>.

When we upgrade to ^15.0.0, we need to take care of this API change in lib/ui/validators.dart.

@ChopinDavid
Copy link
Contributor Author

@laogao You're correct, but this seems like a simple fix. Basically, our validators were previously ValidatorFunctions, a typedef of Map<String, dynamic>? Function(AbstractControl<dynamic> control).

The change to ^15.0.0 is simple. Anything that is currently a ValidatorFunction will turn into an implementation of the new, abstract Validator class. We can override the validate method, which takes an AbstractControl<T> as an argument and returns Map<String, dynamic>?, much like our current ValidatorFunctions.

For example, lib/ui/validators.dart, lines 57-65 could go from looking like this:

res.add((control) {
  if (control.value is String) {
    if (!value.allowDigits! &&
    (control.value as String).contains('.')) {
      return {'allowDigits': value.allowDigits};
    }
  }
  return null;
});

to looking like this:

res.add(AllowDigitsValidator(value.allowDigits!))

where AllowDigitsValidator looks something like this:

class AllowDigitsValidator extends Validator {
  AllowDigitsValidator(this.allowDigits);
  final bool allowDigits;
  @override
  Map<String, dynamic>? validate(AbstractControl control) {
    // TODO: implement validate
    if (control.value is String) {
      if (!allowDigits && (control.value as String).contains('.')) {
        return {'allowDigits': allowDigits};
      }
    }
    return null;
  }
}

and, of course, res is now List<Validator> instead of List<ValidatorFunction>.

@laogao
Copy link
Contributor

laogao commented May 29, 2023

@ChopinDavid

Yup. I'm not saying it's complex or anything. Just that there is a breaking change in 15.0.0 we'll have to deal with.

On a side note, this issue (joanpablo/reactive_forms#377) probably needs more commentary from someone other than me. In case you're interested.

@ChopinDavid
Copy link
Contributor Author

@laogao I'm trying to follow the issue, but there's a lot to take in there. Which issue in our project are we trying to resolve with that proposed change? I'm trying to understand why ValidationMessage.minLength doesn't take care of whatever we need.

@laogao
Copy link
Contributor

laogao commented May 29, 2023

@ChopinDavid

Say we have a text field that records a num, if minLength were used, valid values such as 0, 1, 42, 2.0, etc. would not pass.

@laogao
Copy link
Contributor

laogao commented May 29, 2023

Ultimately this can be circumvented via a custom validator, the approach we've taken in #32 . When (if) that PR gets accepted, we no longer have to supply and maintain our own NonEmptyValidator. It would also have the extra benefit of a single validation message key (in contrast to using different validators according to value types), namely ValidationMessage.requiredNonEmpty, to match isRequired in survey.js.

@ChopinDavid
Copy link
Contributor Author

Closing as not needed.

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 a pull request may close this issue.

2 participants