goxiaoy / flutter_survey_js

Flutter client library for parsing and display surveyjs.io survey
https://goxiaoy.github.io/flutter_survey_js/
MIT License
15 stars 15 forks source link

Upgrade reactive_forms to ^15.0.0 #87

Closed ChopinDavid closed 1 year ago

ChopinDavid commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

@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 (https://github.com/joanpablo/reactive_forms/issues/377) probably needs more commentary from someone other than me. In case you're interested.

ChopinDavid commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

Ultimately this can be circumvented via a custom validator, the approach we've taken in https://github.com/goxiaoy/flutter_survey_js/issues/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 commented 1 year ago

Closing as not needed.