fix(input): $asyncValidators should allow $parsers

[gonzaloruizdevilla]

After resolving the async validators, it should execute all parsers
on the viewValue before comparing it to the currentValue.

Currently, input[type=number] is not recognizing the end of the async validation because
the comparison currentValue === value is comparing a number in currentValue against a string in value
https://github.com/angular/angular.js/blob/master/src/ng/directive/input.js#L1734

[caitp]

This doesn't seem quite right to me, because we've already parsed the value before async validators ever started running. there's probably a bug but I don't think this is the right fix. @matsko?

[gonzaloruizdevilla]

That's the problem. The value is parsed and stored in currentValue. After resolving the async validator, the stored value is compared against the viewValue. No unit test nor e2e tests fails, if I remove this comparison, so, a priori, this would be much faster/better.
It would be something like this:

function resolve(bool) {
      return function() {
        if (ctrl.$pending && ctrl.$pending[validationErrorKey]) {
          pendingCount--;
          delete ctrl.$pending[validationErrorKey];
          ctrl.$setValidity(validationErrorKey, bool);
          if (pendingCount === 0) {
            ctrl.$$clearPending();
            ctrl.$$updateValidModelValue(currentValue);
            ctrl.$$writeModelToScope();
          }
        }
      };
    }

I guess that the comparison is done because the viewValue may change while the asyncValidation is being done. In this case, it maybe be worth storing both the old viewValue and modelValue, and re-executing the parsers only if the viewValue has changed.

[gonzaloruizdevilla]

I've updated the code. Now, it's much cleaner and I think it reflects what it was meant to do. I think the problem was the currentValue argument, it was not clear if it was referencing to model or view. Now, the comparison is between viewvalues, old and new, without the need to execute again the parsers. If all the checks pass, $$updateValidMOdelValue is updated the modelValue.

[caitp]

Thanks =)

[matsko]

This looks really good. I will review it later today and we can get it in tomorrow.

[shahata]

I already submitted a PR for this: #8861

[gonzaloruizdevilla]

@shahata Sorry, I didn't saw your PR. I've reviewed it and I've seen a problem with it. Your are calling in line 1740 ctrl.$$updateValidModelValue(value) with value being ctrl.$viewValue || '', so you are actually setting the updating the model with the viewValue instead of the parsed modelValue.
You can verify this completing your test with: expect(ctrl.$modelValue).toBe('-a');

[shahata]

Yeah, that required a minor fix in my PR. Good catch. Anyway, I like mine a bit more since it makes $$setPending and $$runValidators signatures a bit nicer with less arguments, plus there's a minor performance boost since $$runValidators is not executed in some redundant cases. But truth is both are fine, it doesn't really matter.

[gonzaloruizdevilla]

I have updated my PR with the perf boost. However, I like more @shahata PR. Not only the signatures have less arguments, but in my PR, the arguments modelValue and viewValue have a different nature: modelValue may end being stored in ctrlwhile viewValue is already stored in ctrl. I agree that the only argument that should be passed around is the modelValue that is being checked.

I think that you should put the performance change and test in a separate commit.

[tbosch]

Closing this as the tests pass (after #8941 is now in master). Will add the tests from #8861 to ensure this also works in the future.