bug(formly-form): validateOnModelChange not firing with promise change

[kentcdodds]

@isaac-l noticed that the Matching Two Fields example is broken. After some investigation I found that it works with version 7.1.2, but breaks with version 7.2.0 from this commit by @BarryThePenguin.

@BarryThePenguin, I don't have time right now to look at this, but do you suppose you could give it a look? If not, feel free to assign it back to me. Thanks!

[kentcdodds]

Also, here's a jsbin with the working version. Notice that it references v7.1.2 explicitly.

[kentcdodds]

I think I'll give this one a look.

[kentcdodds]

Here's the original reproduction steps @isaac-l gave the other day

Is there a known issue with http://angular-formly.com/#/example/other/matching-two-fields ? As far as I can tell, it doesn't work as expected For example:

1. Set email to a@b.c
2. Set confirm to a@b.cd (Causes validation error as expected)
3. Set email to a@b.cd (Should be valid, but still shows a validation error) Likewise, setting valid values then changing Email doesn't cause validation to fail

[BarryThePenguin]

Apologies I never responded, haven't had a chance to look into it. Happy to leave this assigned to @kentcdodds

[immortalcy]

While this is working with version 7.1.2 I tried making the changes @BarryThePenguin made to the current version of formly and test it against repeatedSection. It still seems that on repeatedSection version 7.1.2 is not working. So the issue must be something else as well and not only what Barry changed. I am trying to find a work around on this one and keep you posted. @kentcdodds

[nipuna-g]

@immortalcy @kentcdodds

A small thing that I've noted, The example does validate the field when you enter an extra character(Or delete one)

To reproduce this you can,

1. Set email to a@b.c
2. Set confirm to a@b.cd (Causes validation error as expected)
3. Set email to a@b.cd (Should be valid, but still shows a validation error)
4. Set email to a@b.cdd (Should be invalid, but not shows as now shown as valid)

I faced a similar issue when writing a custom validator. It was due to the model being checked 'before' the key up event occurred (Validation is done before the model is changed).

Unfortunately I'm not at all familiar with the Formly core code base to find the bug.

[immortalcy]

@nipuna777 Yes I noticed that as well. Strange behavior but I am not quite sure if this happens because the model is not updated onPress or something. The sure thing is that this "thing" does not apply on repeatedSection at all.

[kentcdodds]

Try this. Add the validateOnModelChange extra to both fields. Then add the allowInvalid ng-model-option to both as well.

The issue is that when the value is invalid, the model value will be undefined, so by doing allowInvalid it will update the model value even when it's invalid. This means that your validator will be able to know what the value of the other field is.

[immortalcy]

@kentcdodds That is really nice to know. However I have updated my non working JSbin and it still doesn't work! FYI here is my stackoverflow question as well.

[schempy]

I made a change to formly-form.js that got things working for me. In the function onModelOrFormStateChange I added parenthesis to validate for a promise:

if ( promise ) {
  promise.then( validate() ) //added parenthesis
} else {
  validate()
}

I'm new to the source code for formly so not sure if this is the correct solution.

[immortalcy]

@schempy I made that change as well.. however that will not fix the problem for a repeatedSection.

[dickp]

I'm using formly v7.3.9

The problem I had with validateOnModelChange is that field.formControl is an array, but the code only considered the plain type version (apiCheck looks for typeOrArrayOf here.)

Here's a proof-of-concept diff:

--- formly.js.orig  2016-02-23 13:15:14.795752158 +0000
+++ formly.js   2016-02-23 13:21:50.467835899 +0000
@@ -2106,11 +2106,21 @@
              field.hide = evalCloseToFormlyExpression(field.hideExpression, val, field, index);
            }
            if (field.extras && field.extras.validateOnModelChange && field.formControl) {
-             var validate = field.formControl.$validate;
-             if (promise) {
-               promise.then(validate);
+            function validateFormControl (formControl) {
+               var validate = formControl.$validate;
+               if (promise) {
+                 promise.then(validate);
+               } else {
+                 validate();
+               }
+            }
+
+             if (_angularFix2['default'].isArray(field.formControl)) {
+               _angularFix2['default'].forEach(field.formControl, function (formControl) {
+                 validateFormControl(formControl);
+               });
              } else {
-               validate();
+               validateFormControl(field.formControl);
              }
            }
          });

[kentcdodds]

Feedback:

1. Looks good
2. Don't use _angularFix2['default'] because that's the transpiled code, just use angular and babel will take care of the rest

Feel free to open a PR.

[dickp]

I've opened a PR. It fixes my problem, hopefully it helps for the others watching this issue.

[gillchristian]

Since #641 was merged, this issue could be closed.

[kentcdodds]

:+1: @gillchristian!

[BarryThePenguin]

This is still occurring, even in 8.0.0

[pseudo-su]

The problem I have is that formlyField.runExpressions returns the $timeout promise but it should be resolving after all the field values are set.

here's what I did to fix.

[BarryThePenguin]

Thanks @psuedoSudo, that definitely gets us closer to a solution!

I'm a little worried about modifying that promises array inside the forEach loop. Maybe we need to return a promise that is resolved after the $timeout and each of the fields have been set?

[kwypchlo]

@psuedoSudo if you return $q.all(promises) at the moment of the return, promises variable will be equal to empty array thus anything that attaches itself with .then to the result of this function will be resolved at the moment of declaration with empty argument. This is because $timeout will wait for the next event loop to be executed and by then, $q.all([]) will already be resolved and anything you push to the promises array will be ignored. Correct me if I'm wrong please :)

[BarryThePenguin]

@psuedoSudo, if you put together a pull request we can get it reviewed and merged. If you need any help please let me know. If you @ me on gitter I can help out.

I'm going to put together a solution too, just so I know what works 👍

[pseudo-su]

Completely correct @kwypchlo, bit of a brain fart. The issue should still be that the $timeout promise resolves before all the fields are set. I'll update it now

[Rybbow]

I submitted a pull-request #728, that I guess fixes the bug mentioned in this issue. Let me know what you think.