Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

fix(ngModel): fix issues when parserName is same as validator key #10850

Closed
wants to merge 2 commits into from
Closed

fix(ngModel): fix issues when parserName is same as validator key #10850

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5d62a95
fix(ngModel): fix issues when parserName is same as validator key
Narretz Jan 20, 2015
076b39b
test(ngModel): refactor the test
Narretz Feb 8, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions src/ng/directive/ngModel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,7 @@ var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$
this.$dirty = false;
this.$valid = true;
this.$invalid = false;
this.$$parseError = false;
this.$error = {}; // keep invalid keys here
this.$$success = {}; // keep valid keys here
this.$pending = undefined; // keep pending keys here
Expand Down Expand Up @@ -518,7 +519,7 @@ var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$

// Check if the there's a parse error, so we don't unset it accidentially
var parserName = ctrl.$$parserName || 'parse';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This variable doesn't seem to be used now. What does this mean for the default of 'parse'?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that the errorKey variable sets the default below on line 563

var parserValid = ctrl.$error[parserName] ? false : undefined;
var parserValid = ctrl.$$parseError ? false : undefined;

var prevValid = ctrl.$valid;
var prevModelValue = ctrl.$modelValue;
Expand Down Expand Up @@ -563,17 +564,21 @@ var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$
if (parseValid === undefined) {
setValidity(errorKey, null);
} else {
setValidity(errorKey, parseValid);
if (!parseValid) {
forEach(ctrl.$validators, function(v, name) {
setValidity(name, null);
});
forEach(ctrl.$asyncValidators, function(v, name) {
setValidity(name, null);
});
return false;
ctrl.$$parseError = true;
}
// Set the parse error last, to prevent unsetting it, should a $validators key == parserName
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment doesn't read very well. Also if we remove the unused parserName variable then this makes less sense.

setValidity(errorKey, parseValid);
return parseValid;
}

ctrl.$$parseError = false;
return true;
}

Expand Down Expand Up @@ -814,6 +819,7 @@ var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$
// TODO(perf): why not move this to the action fn?
if (modelValue !== ctrl.$modelValue) {
ctrl.$modelValue = ctrl.$$rawModelValue = modelValue;
ctrl.$$parseError = false;

var formatters = ctrl.$formatters,
idx = formatters.length;
Expand Down
74 changes: 74 additions & 0 deletions test/ng/directive/ngModelSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1221,6 +1221,80 @@ describe('ngModel', function() {
expect(ctrl.$validators.mock).toHaveBeenCalledWith('a', 'ab');
expect(ctrl.$validators.mock.calls.length).toEqual(2);
});

it('should validate correctly when $parser name equals $validator key', function() {

var testValid, otherValid, parseValid;

ctrl.$validators.test = function(value) {
return testValid;
};

ctrl.$validators.other = function(value) {
return otherValid;
};

ctrl.$$parserName = 'test';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think ctrl.$$parserName was a bad idea in the first place. We ought to refactor this in general if possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any ideas? It's currently a private API, but we rely on it for the name of the error key. A separate function, $setParseError(name) might be possible that is called inside the parse error. Although I think we have to stick with return undefined denoting a parse error, bad as it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also allow returning an object that contains parser name, error state, and return value. This would allow returning undefined as a valid value, as well as get rid of $$parserName and expose an API to actually set the parser name.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about we use the name on the parser function? This would let us provide a named function for a parse and get a free name, or we can specify it explicitly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just noticed that name is not supported in IE9+ - we'd have to polyfill it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, how about we do something like: parser.name || parser.$$name or something to allow people to specify the name explicitly if they need to support IE9

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's all IEs, up to IE11, so I don't think it's an option.

ctrl.$parsers.push(function(value) {
return parseValid ? true : undefined;
});


testValid = otherValid = parseValid = false;
scope.$apply('value = "allInvalid"');
expect(scope.value).toBe('allInvalid');
expect(ctrl.$error).toEqual({test: true, other: true});

ctrl.$validate();
expect(scope.value).toBe('allInvalid');
expect(ctrl.$error).toEqual({test: true, other: true});

ctrl.$setViewValue('stillAllInvalid');
expect(scope.value).toBeUndefined();
expect(ctrl.$error).toEqual({test: true});

ctrl.$validate();
expect(scope.value).toBeUndefined();
expect(ctrl.$error).toEqual({test: true});


parseValid = true;
scope.$apply('value = "parseValidAndValidatorInvalid"');
expect(scope.value).toBe('parseValidAndValidatorInvalid');
expect(ctrl.$error).toEqual({test: true, other: true});

ctrl.$validate();
expect(scope.value).toBe('parseValidAndValidatorInvalid');
expect(ctrl.$error).toEqual({test: true, other: true});

ctrl.$setViewValue('stillParseValidAndValidatorInvalid');
expect(scope.value).toBeUndefined();
expect(ctrl.$error).toEqual({test: true, other: true});

ctrl.$validate();
expect(scope.value).toBeUndefined();
expect(ctrl.$error).toEqual({test: true, other: true});


parseValid = false;
testValid = otherValid = true;
scope.$apply('value = "parseInvalidAndValidatorValid"');
expect(scope.value).toBe('parseInvalidAndValidatorValid');
expect(ctrl.$error).toEqual({});

ctrl.$validate();
expect(scope.value).toBe('parseInvalidAndValidatorValid');
expect(ctrl.$error).toEqual({});

ctrl.$setViewValue('stillParseInvalidAndValidatorValid');
expect(scope.value).toBeUndefined();
expect(ctrl.$error).toEqual({test: true});

ctrl.$validate();
expect(scope.value).toBeUndefined();
expect(ctrl.$error).toEqual({test: true});
});

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test seems overly complicated. Couldn't we replace the parser and validators with simple functions, like:

  • parser -> isEven(value)
  • test -> isGreaterThanTen(value)
  • other -> isOdd(value)

Then we don't have to grok as many variables - just simply send in the following values:

  • 1
  • 2
  • 11
  • 12

And check that we get the correct validation?

});
});

Expand Down