artflutter / reactive_forms_generator

Other
89 stars 22 forks source link

didUpdateWidget should not reassign the form #143

Closed BenjiFarquhar closed 9 months ago

BenjiFarquhar commented 9 months ago

I've noticed reassigning the formModel in didUpdateWidget is problematic for anything other than example forms. Subscriptions are lost, and other weird things happen by replacing the form with a completely new form. Forms should live as long as the widget lives because they have too many tentacles into other components via subscriptions, which get lost when the form is reassigned. Validators start validating the wrong form controls that are no longer in use. reactive_forms_lbc widgets start acting strange. Forms are not immutable objects, nor should they be, so they shouldn't be treated like it by reassigning them.

Solution: Mutate the form. A quick and easy solution is to use formModel.updateValue, where you reassign the form. The updateValue value param will need a null-aware operator. You can then get rid of _formModel.form.markAsDisabled() and widget.initState?.call(context, _formModel) as none of the original form state will be lost lost.

E.g.:

  @override
  void didUpdateWidget(covariant DetailsFormBuilder oldWidget) {
    if (widget.model != oldWidget.model) {
      _formModel = DetailsForm(DetailsForm.formElements(widget.model), null);

      if (_formModel.form.disabled) {
        _formModel.form.markAsDisabled();
      }

      widget.initState?.call(context, _formModel);
    }

    super.didUpdateWidget(oldWidget);
  }

Will become:

  @override
  void didUpdateWidget(covariant DetailsFormBuilder oldWidget) {
    if (widget.model != oldWidget.model) {
      _formModel.updateValue(widget.model);
    }

    super.didUpdateWidget(oldWidget);
  }

and

  @override
  void updateValue(
    Details value, {
    bool updateParent = true,
    bool emitEvent = true,
  }) =>
      form.updateValue(DetailsForm.formElements(value).rawValue,
          updateParent: updateParent, emitEvent: emitEvent);

Will become:

  @override
  void updateValue(
    Details? value, {
    bool updateParent = true,
    bool emitEvent = true,
  }) =>
      form.updateValue(DetailsForm.formElements(value).rawValue,
          updateParent: updateParent, emitEvent: emitEvent);
vasilich6107 commented 9 months ago

Hi. Thanks for this finding and for contribution This definitely makes sense.

vasilich6107 commented 9 months ago

released

github-actions[bot] commented 9 months ago

Hi @BenjiFarquhar! Your issue has been closed. If we were helpful don't forget to star the repo.

Please check our reactive_forms_widget package

We would appreciate sponsorship subscription or one time donation https://github.com/sponsors/artflutter