From a81295479085d0643f3452ccbe7d510761c19910 Mon Sep 17 00:00:00 2001 From: Bob Chao Date: Tue, 27 Sep 2016 01:19:37 -0700 Subject: [PATCH 1/5] Demonstrate tests for issue #14734 --- src/ng/directive/ngModel.js | 1 + test/ng/directive/ngModelSpec.js | 68 ++++++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+) diff --git a/src/ng/directive/ngModel.js b/src/ng/directive/ngModel.js index 55310adceddd..c77b454c3d59 100644 --- a/src/ng/directive/ngModel.js +++ b/src/ng/directive/ngModel.js @@ -604,6 +604,7 @@ NgModelController.prototype = { var syncValidatorsValid = true; forEach(that.$validators, function(validator, name) { var result = validator(modelValue, viewValue); + // var result = Boolean(validator(modelValue, viewValue)); syncValidatorsValid = syncValidatorsValid && result; setValidity(name, result); }); diff --git a/test/ng/directive/ngModelSpec.js b/test/ng/directive/ngModelSpec.js index 37c633f13c97..b4523b41f568 100644 --- a/test/ng/directive/ngModelSpec.js +++ b/test/ng/directive/ngModelSpec.js @@ -847,6 +847,74 @@ describe('ngModel', function() { expect(ctrl.$valid).toBe(true); }); + // See also the test on L886. + fit('should demonstrate how synchronous validators work now', function() { + var curry = function(v) { + return function() { + return v; + }; + }; + + var tester = function(v, e) { + ctrl.$modelValue = undefined; + ctrl.$validators.a = curry(v); + + ctrl.$validate(); + expect(ctrl.$valid).toBe(e); + }; + + // False tests + tester(false, false); + tester(undefined, undefined); + tester(null, true); + tester(0, true); + tester(NaN, true); + tester('', true); + + // True tests + tester(true, true); + tester(1, true); + tester('0', true); + tester('false', true); + tester([], true); + tester({}, true); + }); + + // Flip this test with the one above (L851) as well as line L606 with L607 in `ngModel.js` + // The differences between the two would likely being a breaking change + // Though the feature it is breaking was undocumented. + xit('should demonstrate how synchronous validators will work after this change', function() { + var curry = function(v) { + return function() { + return v; + }; + }; + + var tester = function(v, e) { + ctrl.$modelValue = undefined; + ctrl.$validators.a = curry(v); + + ctrl.$validate(); + expect(ctrl.$valid).toBe(e); + }; + + // False tests + tester(false, false); + tester(undefined, false); + tester(null, false); + tester(0, false); + tester(NaN, false); + tester('', false); + + // True tests + tester(true, true); + tester(1, true); + tester('0', true); + tester('false', true); + tester([], true); + tester({}, true); + }); + it('should register invalid validations on the $error object', function() { var curry = function(v) { From 1a1d0fb3dac03a02d1f607c7c119f7964d416040 Mon Sep 17 00:00:00 2001 From: BobChao87 Date: Mon, 3 Oct 2016 01:58:36 -0700 Subject: [PATCH 2/5] fix(ngModel): treat synchronous validators as boolean always Change synchronous validators to convert the return to boolean value. Prevent unexpected behavior when returning `undefined`. Closes #14734 BREAKING CHANGE: Previously, only a literal `false` return would resolve as the synchronous validator failing. Now, all traditionally false JavaScript values are treated as failing the validator, as one would naturally expect. To migrate, make sure your synchronous validators are returning either a literal `true` or a literal `false` value. For most code, we expect this to already be the case. Only a very small subset of projects will affected. Namely, anyone using `undefined` or any falsy value as a return will now see their validation failing, whereas previously falsy values other than `undefined` would have been seen as passing and `undefined` would have been seen as pending. --- src/ng/directive/ngModel.js | 3 +-- test/ng/directive/ngModelSpec.js | 38 +------------------------------- 2 files changed, 2 insertions(+), 39 deletions(-) diff --git a/src/ng/directive/ngModel.js b/src/ng/directive/ngModel.js index c77b454c3d59..a74e8a4bffae 100644 --- a/src/ng/directive/ngModel.js +++ b/src/ng/directive/ngModel.js @@ -603,8 +603,7 @@ NgModelController.prototype = { function processSyncValidators() { var syncValidatorsValid = true; forEach(that.$validators, function(validator, name) { - var result = validator(modelValue, viewValue); - // var result = Boolean(validator(modelValue, viewValue)); + var result = Boolean(validator(modelValue, viewValue)); syncValidatorsValid = syncValidatorsValid && result; setValidity(name, result); }); diff --git a/test/ng/directive/ngModelSpec.js b/test/ng/directive/ngModelSpec.js index b4523b41f568..b96feaeb6734 100644 --- a/test/ng/directive/ngModelSpec.js +++ b/test/ng/directive/ngModelSpec.js @@ -847,43 +847,7 @@ describe('ngModel', function() { expect(ctrl.$valid).toBe(true); }); - // See also the test on L886. - fit('should demonstrate how synchronous validators work now', function() { - var curry = function(v) { - return function() { - return v; - }; - }; - - var tester = function(v, e) { - ctrl.$modelValue = undefined; - ctrl.$validators.a = curry(v); - - ctrl.$validate(); - expect(ctrl.$valid).toBe(e); - }; - - // False tests - tester(false, false); - tester(undefined, undefined); - tester(null, true); - tester(0, true); - tester(NaN, true); - tester('', true); - - // True tests - tester(true, true); - tester(1, true); - tester('0', true); - tester('false', true); - tester([], true); - tester({}, true); - }); - - // Flip this test with the one above (L851) as well as line L606 with L607 in `ngModel.js` - // The differences between the two would likely being a breaking change - // Though the feature it is breaking was undocumented. - xit('should demonstrate how synchronous validators will work after this change', function() { + it('should treat all responses as boolean for synchronous validators', function() { var curry = function(v) { return function() { return v; From 7e4bf60385d77f3197a91c54e8a84235be2de5b3 Mon Sep 17 00:00:00 2001 From: BobChao87 Date: Tue, 4 Oct 2016 02:22:40 -0700 Subject: [PATCH 3/5] fix(ngModel): treat synchronous validators as boolean always Change synchronous validators to convert the return to boolean value. Prevent unexpected behavior when returning `undefined`. Closes #14734 BREAKING CHANGE: Previously, only a literal `false` return would resolve as the synchronous validator failing. Now, all traditionally false JavaScript values are treated as failing the validator, as one would naturally expect. Specifically, the values `0` (the number zero), `null`, `NaN` and `''` (the empty string) used to considered valid (passing) and they are now considered invalid (failing). The value `undefined` was treated similarly to a pending asynchronous validator, causing the validation to be pending. `undefined` is also now considered invalid. To migrate, make sure your synchronous validators are returning either a literal `true` or a literal `false` value. For most code, we expect this to already be the case. Only a very small subset of projects will be affected. Namely, anyone using `undefined` or any falsy value as a return will now see their validation failing, whereas previously falsy values other than `undefined` would have been seen as passing and `undefined` would have been seen as pending. --- test/ng/directive/ngModelSpec.js | 42 ++++++++++++++++---------------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/test/ng/directive/ngModelSpec.js b/test/ng/directive/ngModelSpec.js index b96feaeb6734..8261358bf428 100644 --- a/test/ng/directive/ngModelSpec.js +++ b/test/ng/directive/ngModelSpec.js @@ -847,36 +847,36 @@ describe('ngModel', function() { expect(ctrl.$valid).toBe(true); }); - it('should treat all responses as boolean for synchronous validators', function() { - var curry = function(v) { - return function() { - return v; - }; - }; - - var tester = function(v, e) { + fit('should treat all responses as boolean for synchronous validators', function() { + // var curry = function(v) { + // return function() { + // return v; + // }; + // }; + + var expectValid = function(v, e) { ctrl.$modelValue = undefined; - ctrl.$validators.a = curry(v); + ctrl.$validators.a = valueFn(v); ctrl.$validate(); expect(ctrl.$valid).toBe(e); }; // False tests - tester(false, false); - tester(undefined, false); - tester(null, false); - tester(0, false); - tester(NaN, false); - tester('', false); + expectValid(false, false); + expectValid(undefined, false); + expectValid(null, false); + expectValid(0, false); + expectValid(NaN, false); + expectValid('', false); // True tests - tester(true, true); - tester(1, true); - tester('0', true); - tester('false', true); - tester([], true); - tester({}, true); + expectValid(true, true); + expectValid(1, true); + expectValid('0', true); + expectValid('false', true); + expectValid([], true); + expectValid({}, true); }); From 5ce305343891a3170a5f5ca500d284f5c6e8b9b7 Mon Sep 17 00:00:00 2001 From: BobChao87 Date: Tue, 4 Oct 2016 02:30:36 -0700 Subject: [PATCH 4/5] fix(ngModel): treat synchronous validators as boolean always Change synchronous validators to convert the return to boolean value. Prevent unexpected behavior when returning `undefined`. Closes #14734 BREAKING CHANGE: Previously, only a literal `false` return would resolve as the synchronous validator failing. Now, all traditionally false JavaScript values are treated as failing the validator, as one would naturally expect. Specifically, the values `0` (the number zero), `null`, `NaN` and `''` (the empty string) used to considered valid (passing) and they are now considered invalid (failing). The value `undefined` was treated similarly to a pending asynchronous validator, causing the validation to be pending. `undefined` is also now considered invalid. To migrate, make sure your synchronous validators are returning either a literal `true` or a literal `false` value. For most code, we expect this to already be the case. Only a very small subset of projects will be affected. Namely, anyone using `undefined` or any falsy value as a return will now see their validation failing, whereas previously falsy values other than `undefined` would have been seen as passing and `undefined` would have been seen as pending. --- test/ng/directive/ngModelSpec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/ng/directive/ngModelSpec.js b/test/ng/directive/ngModelSpec.js index 8261358bf428..75e97152ec1c 100644 --- a/test/ng/directive/ngModelSpec.js +++ b/test/ng/directive/ngModelSpec.js @@ -847,7 +847,7 @@ describe('ngModel', function() { expect(ctrl.$valid).toBe(true); }); - fit('should treat all responses as boolean for synchronous validators', function() { + it('should treat all responses as boolean for synchronous validators', function() { // var curry = function(v) { // return function() { // return v; From 64c6d39a0e029e0b4446521dce9c230ccdf76fb4 Mon Sep 17 00:00:00 2001 From: BobChao87 Date: Tue, 4 Oct 2016 02:40:15 -0700 Subject: [PATCH 5/5] fix(ngModel): treat synchronous validators as boolean always Change synchronous validators to convert the return to boolean value. Prevent unexpected behavior when returning `undefined`. Closes #14734 BREAKING CHANGE: Previously, only a literal `false` return would resolve as the synchronous validator failing. Now, all traditionally false JavaScript values are treated as failing the validator, as one would naturally expect. Specifically, the values `0` (the number zero), `null`, `NaN` and `''` (the empty string) used to considered valid (passing) and they are now considered invalid (failing). The value `undefined` was treated similarly to a pending asynchronous validator, causing the validation to be pending. `undefined` is also now considered invalid. To migrate, make sure your synchronous validators are returning either a literal `true` or a literal `false` value. For most code, we expect this to already be the case. Only a very small subset of projects will be affected. Namely, anyone using `undefined` or any falsy value as a return will now see their validation failing, whereas previously falsy values other than `undefined` would have been seen as passing and `undefined` would have been seen as pending. --- test/ng/directive/ngModelSpec.js | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/test/ng/directive/ngModelSpec.js b/test/ng/directive/ngModelSpec.js index 75e97152ec1c..b21f21baeea1 100644 --- a/test/ng/directive/ngModelSpec.js +++ b/test/ng/directive/ngModelSpec.js @@ -848,18 +848,12 @@ describe('ngModel', function() { }); it('should treat all responses as boolean for synchronous validators', function() { - // var curry = function(v) { - // return function() { - // return v; - // }; - // }; - - var expectValid = function(v, e) { + var expectValid = function(value, expected) { ctrl.$modelValue = undefined; - ctrl.$validators.a = valueFn(v); + ctrl.$validators.a = valueFn(value); ctrl.$validate(); - expect(ctrl.$valid).toBe(e); + expect(ctrl.$valid).toBe(expected); }; // False tests