From 985a484bdc8e26c87ee0f2db48333dbdcf6c84a5 Mon Sep 17 00:00:00 2001 From: Troy DeMonbreun Date: Tue, 22 Mar 2016 04:07:06 +0000 Subject: [PATCH 1/6] Fix for 5468: Validate proptype definitions sooner Added typeCheckWarn() func and updated the oneOf/oneOfType tests Added __DEV__ warning for invalid oneOf/OneOfType args --- .../classic/types/ReactPropTypes.js | 7 +++++++ .../types/__tests__/ReactPropTypes-test.js | 19 +++++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/src/isomorphic/classic/types/ReactPropTypes.js b/src/isomorphic/classic/types/ReactPropTypes.js index 4d9ac1d846107..f8cc3f9fe4a97 100644 --- a/src/isomorphic/classic/types/ReactPropTypes.js +++ b/src/isomorphic/classic/types/ReactPropTypes.js @@ -16,6 +16,7 @@ var ReactPropTypeLocationNames = require('ReactPropTypeLocationNames'); var emptyFunction = require('emptyFunction'); var getIteratorFn = require('getIteratorFn'); +var warning = require('warning'); /** * Collection of methods that allow declaration and validation of props that are @@ -226,6 +227,9 @@ function createInstanceTypeChecker(expectedClass) { function createEnumTypeChecker(expectedValues) { if (!Array.isArray(expectedValues)) { + if (__DEV__) { + warning(false, 'Invalid argument supplied to oneOf, expected an instance of array.'); + } return createChainableTypeChecker(function() { return new Error( `Invalid argument supplied to oneOf, expected an instance of array.` @@ -288,6 +292,9 @@ function createObjectOfTypeChecker(typeChecker) { function createUnionTypeChecker(arrayOfTypeCheckers) { if (!Array.isArray(arrayOfTypeCheckers)) { + if (__DEV__) { + warning(false, 'Invalid argument supplied to oneOfType, expected an instance of array.'); + } return createChainableTypeChecker(function() { return new Error( `Invalid argument supplied to oneOfType, expected an instance of array.` diff --git a/src/isomorphic/classic/types/__tests__/ReactPropTypes-test.js b/src/isomorphic/classic/types/__tests__/ReactPropTypes-test.js index 14559720aeb0d..817691ae10a38 100644 --- a/src/isomorphic/classic/types/__tests__/ReactPropTypes-test.js +++ b/src/isomorphic/classic/types/__tests__/ReactPropTypes-test.js @@ -45,6 +45,13 @@ function typeCheckPass(declaration, value) { expect(error).toBe(null); } +function typeCheckWarn(propTypeFunc, message) { + spyOn(console, ['error']); + propTypeFunc(); + expect(console.error).toHaveBeenCalled(); + expect(console.error.argsForCall[0][0]).toContain(message); +} + describe('ReactPropTypes', function() { beforeEach(function() { PropTypes = require('ReactPropTypes'); @@ -591,6 +598,12 @@ describe('ReactPropTypes', function() { describe('OneOf Types', function() { it('should fail for invalid argument', function() { + typeCheckWarn( + function() { + PropTypes.oneOf('red', 'blue'); + }, + 'Invalid argument supplied to oneOf, expected an instance of array.' + ); typeCheckFail( PropTypes.oneOf('red', 'blue'), 'red', @@ -652,6 +665,12 @@ describe('ReactPropTypes', function() { describe('Union Types', function() { it('should fail for invalid argument', function() { + typeCheckWarn( + function() { + PropTypes.oneOfType(PropTypes.string, PropTypes.number); + }, + 'Invalid argument supplied to oneOfType, expected an instance of array.' + ); typeCheckFail( PropTypes.oneOfType(PropTypes.string, PropTypes.number), 'red', From 72e5ada9ba79f5038164da01d122d4b8bdfa25e8 Mon Sep 17 00:00:00 2001 From: Troy DeMonbreun Date: Mon, 27 Jun 2016 12:43:24 -0500 Subject: [PATCH 2/6] Suppress redundant error on warn; typeCheckWarn() removed --- .../classic/types/ReactPropTypes.js | 22 +++++----- .../types/__tests__/ReactPropTypes-test.js | 41 ++++++------------- 2 files changed, 24 insertions(+), 39 deletions(-) diff --git a/src/isomorphic/classic/types/ReactPropTypes.js b/src/isomorphic/classic/types/ReactPropTypes.js index f8cc3f9fe4a97..880abc539b052 100644 --- a/src/isomorphic/classic/types/ReactPropTypes.js +++ b/src/isomorphic/classic/types/ReactPropTypes.js @@ -229,12 +229,13 @@ function createEnumTypeChecker(expectedValues) { if (!Array.isArray(expectedValues)) { if (__DEV__) { warning(false, 'Invalid argument supplied to oneOf, expected an instance of array.'); + } else { + return createChainableTypeChecker(function() { + return new Error( + `Invalid argument supplied to oneOf, expected an instance of array.` + ); + }); } - return createChainableTypeChecker(function() { - return new Error( - `Invalid argument supplied to oneOf, expected an instance of array.` - ); - }); } function validate(props, propName, componentName, location, propFullName) { @@ -294,12 +295,13 @@ function createUnionTypeChecker(arrayOfTypeCheckers) { if (!Array.isArray(arrayOfTypeCheckers)) { if (__DEV__) { warning(false, 'Invalid argument supplied to oneOfType, expected an instance of array.'); + } else { + return createChainableTypeChecker(function() { + return new Error( + `Invalid argument supplied to oneOfType, expected an instance of array.` + ); + }); } - return createChainableTypeChecker(function() { - return new Error( - `Invalid argument supplied to oneOfType, expected an instance of array.` - ); - }); } function validate(props, propName, componentName, location, propFullName) { diff --git a/src/isomorphic/classic/types/__tests__/ReactPropTypes-test.js b/src/isomorphic/classic/types/__tests__/ReactPropTypes-test.js index 817691ae10a38..98cd1650b0019 100644 --- a/src/isomorphic/classic/types/__tests__/ReactPropTypes-test.js +++ b/src/isomorphic/classic/types/__tests__/ReactPropTypes-test.js @@ -45,13 +45,6 @@ function typeCheckPass(declaration, value) { expect(error).toBe(null); } -function typeCheckWarn(propTypeFunc, message) { - spyOn(console, ['error']); - propTypeFunc(); - expect(console.error).toHaveBeenCalled(); - expect(console.error.argsForCall[0][0]).toContain(message); -} - describe('ReactPropTypes', function() { beforeEach(function() { PropTypes = require('ReactPropTypes'); @@ -598,17 +591,12 @@ describe('ReactPropTypes', function() { describe('OneOf Types', function() { it('should fail for invalid argument', function() { - typeCheckWarn( - function() { - PropTypes.oneOf('red', 'blue'); - }, - 'Invalid argument supplied to oneOf, expected an instance of array.' - ); - typeCheckFail( - PropTypes.oneOf('red', 'blue'), - 'red', - 'Invalid argument supplied to oneOf, expected an instance of array.' - ); + spyOn(console, ['error']); + + PropTypes.oneOf('red', 'blue'); + + expect(console.error).toHaveBeenCalled(); + expect(console.error.calls.argsFor(0)[0]).toContain('Invalid argument supplied to oneOf, expected an instance of array.'); }); it('should warn for invalid values', function() { @@ -665,17 +653,12 @@ describe('ReactPropTypes', function() { describe('Union Types', function() { it('should fail for invalid argument', function() { - typeCheckWarn( - function() { - PropTypes.oneOfType(PropTypes.string, PropTypes.number); - }, - 'Invalid argument supplied to oneOfType, expected an instance of array.' - ); - typeCheckFail( - PropTypes.oneOfType(PropTypes.string, PropTypes.number), - 'red', - 'Invalid argument supplied to oneOfType, expected an instance of array.' - ); + spyOn(console, ['error']); + + PropTypes.oneOfType(PropTypes.string, PropTypes.number); + + expect(console.error).toHaveBeenCalled(); + expect(console.error.calls.argsFor(0)[0]).toContain('Invalid argument supplied to oneOfType, expected an instance of array.'); }); it('should warn if none of the types are valid', function() { From 5a1f598a7b5871421b2e8cfabc195e6032840865 Mon Sep 17 00:00:00 2001 From: Troy DeMonbreun Date: Mon, 27 Jun 2016 13:09:17 -0500 Subject: [PATCH 3/6] Return no-op --- src/isomorphic/classic/types/ReactPropTypes.js | 2 ++ .../classic/types/__tests__/ReactPropTypes-test.js | 6 ++++-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/isomorphic/classic/types/ReactPropTypes.js b/src/isomorphic/classic/types/ReactPropTypes.js index 880abc539b052..84549b9171f79 100644 --- a/src/isomorphic/classic/types/ReactPropTypes.js +++ b/src/isomorphic/classic/types/ReactPropTypes.js @@ -229,6 +229,7 @@ function createEnumTypeChecker(expectedValues) { if (!Array.isArray(expectedValues)) { if (__DEV__) { warning(false, 'Invalid argument supplied to oneOf, expected an instance of array.'); + return function() {}; } else { return createChainableTypeChecker(function() { return new Error( @@ -295,6 +296,7 @@ function createUnionTypeChecker(arrayOfTypeCheckers) { if (!Array.isArray(arrayOfTypeCheckers)) { if (__DEV__) { warning(false, 'Invalid argument supplied to oneOfType, expected an instance of array.'); + return function() {}; } else { return createChainableTypeChecker(function() { return new Error( diff --git a/src/isomorphic/classic/types/__tests__/ReactPropTypes-test.js b/src/isomorphic/classic/types/__tests__/ReactPropTypes-test.js index 98cd1650b0019..5c95679cb25b4 100644 --- a/src/isomorphic/classic/types/__tests__/ReactPropTypes-test.js +++ b/src/isomorphic/classic/types/__tests__/ReactPropTypes-test.js @@ -596,7 +596,8 @@ describe('ReactPropTypes', function() { PropTypes.oneOf('red', 'blue'); expect(console.error).toHaveBeenCalled(); - expect(console.error.calls.argsFor(0)[0]).toContain('Invalid argument supplied to oneOf, expected an instance of array.'); + expect(console.error.calls.argsFor(0)[0]) + .toContain('Invalid argument supplied to oneOf, expected an instance of array.'); }); it('should warn for invalid values', function() { @@ -658,7 +659,8 @@ describe('ReactPropTypes', function() { PropTypes.oneOfType(PropTypes.string, PropTypes.number); expect(console.error).toHaveBeenCalled(); - expect(console.error.calls.argsFor(0)[0]).toContain('Invalid argument supplied to oneOfType, expected an instance of array.'); + expect(console.error.calls.argsFor(0)[0]) + .toContain('Invalid argument supplied to oneOfType, expected an instance of array.'); }); it('should warn if none of the types are valid', function() { From d22581631bc50a7df30a1809085ee03fcded3b69 Mon Sep 17 00:00:00 2001 From: Troy DeMonbreun Date: Mon, 27 Jun 2016 15:39:18 -0500 Subject: [PATCH 4/6] Using emptyFunction module for consistency --- src/isomorphic/classic/types/ReactPropTypes.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/isomorphic/classic/types/ReactPropTypes.js b/src/isomorphic/classic/types/ReactPropTypes.js index 84549b9171f79..b0b8535d5b1f3 100644 --- a/src/isomorphic/classic/types/ReactPropTypes.js +++ b/src/isomorphic/classic/types/ReactPropTypes.js @@ -229,7 +229,7 @@ function createEnumTypeChecker(expectedValues) { if (!Array.isArray(expectedValues)) { if (__DEV__) { warning(false, 'Invalid argument supplied to oneOf, expected an instance of array.'); - return function() {}; + return emptyFunction.thatReturnsNull; } else { return createChainableTypeChecker(function() { return new Error( @@ -296,7 +296,7 @@ function createUnionTypeChecker(arrayOfTypeCheckers) { if (!Array.isArray(arrayOfTypeCheckers)) { if (__DEV__) { warning(false, 'Invalid argument supplied to oneOfType, expected an instance of array.'); - return function() {}; + return emptyFunction.thatReturnsNull; } else { return createChainableTypeChecker(function() { return new Error( From e1c7e55e23954f9fd34a94eb656a2fa33a6cc57d Mon Sep 17 00:00:00 2001 From: Troy DeMonbreun Date: Mon, 27 Jun 2016 17:00:58 -0500 Subject: [PATCH 5/6] Remove createChainableTypeChecker() call --- .../classic/types/ReactPropTypes.js | 24 ++++--------------- 1 file changed, 4 insertions(+), 20 deletions(-) diff --git a/src/isomorphic/classic/types/ReactPropTypes.js b/src/isomorphic/classic/types/ReactPropTypes.js index b0b8535d5b1f3..9b419cc769594 100644 --- a/src/isomorphic/classic/types/ReactPropTypes.js +++ b/src/isomorphic/classic/types/ReactPropTypes.js @@ -227,16 +227,8 @@ function createInstanceTypeChecker(expectedClass) { function createEnumTypeChecker(expectedValues) { if (!Array.isArray(expectedValues)) { - if (__DEV__) { - warning(false, 'Invalid argument supplied to oneOf, expected an instance of array.'); - return emptyFunction.thatReturnsNull; - } else { - return createChainableTypeChecker(function() { - return new Error( - `Invalid argument supplied to oneOf, expected an instance of array.` - ); - }); - } + warning(false, 'Invalid argument supplied to oneOf, expected an instance of array.'); + return emptyFunction.thatReturnsNull; } function validate(props, propName, componentName, location, propFullName) { @@ -294,16 +286,8 @@ function createObjectOfTypeChecker(typeChecker) { function createUnionTypeChecker(arrayOfTypeCheckers) { if (!Array.isArray(arrayOfTypeCheckers)) { - if (__DEV__) { - warning(false, 'Invalid argument supplied to oneOfType, expected an instance of array.'); - return emptyFunction.thatReturnsNull; - } else { - return createChainableTypeChecker(function() { - return new Error( - `Invalid argument supplied to oneOfType, expected an instance of array.` - ); - }); - } + warning(false, 'Invalid argument supplied to oneOfType, expected an instance of array.'); + return emptyFunction.thatReturnsNull; } function validate(props, propName, componentName, location, propFullName) { From 1e6ab813b5d2c3c291519aaf04c0dd20e5f84b43 Mon Sep 17 00:00:00 2001 From: Troy DeMonbreun Date: Tue, 28 Jun 2016 14:24:24 -0500 Subject: [PATCH 6/6] Adjust test to assert type check passes when warned --- .../classic/types/__tests__/ReactPropTypes-test.js | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/isomorphic/classic/types/__tests__/ReactPropTypes-test.js b/src/isomorphic/classic/types/__tests__/ReactPropTypes-test.js index 5c95679cb25b4..4e787857c76e3 100644 --- a/src/isomorphic/classic/types/__tests__/ReactPropTypes-test.js +++ b/src/isomorphic/classic/types/__tests__/ReactPropTypes-test.js @@ -590,14 +590,16 @@ describe('ReactPropTypes', function() { }); describe('OneOf Types', function() { - it('should fail for invalid argument', function() { - spyOn(console, ['error']); + it('should warn but not error for invalid argument', function() { + spyOn(console, 'error'); PropTypes.oneOf('red', 'blue'); expect(console.error).toHaveBeenCalled(); expect(console.error.calls.argsFor(0)[0]) .toContain('Invalid argument supplied to oneOf, expected an instance of array.'); + + typeCheckPass(PropTypes.oneOf('red', 'blue'), 'red'); }); it('should warn for invalid values', function() { @@ -653,14 +655,16 @@ describe('ReactPropTypes', function() { }); describe('Union Types', function() { - it('should fail for invalid argument', function() { - spyOn(console, ['error']); + it('should warn but not error for invalid argument', function() { + spyOn(console, 'error'); PropTypes.oneOfType(PropTypes.string, PropTypes.number); expect(console.error).toHaveBeenCalled(); expect(console.error.calls.argsFor(0)[0]) .toContain('Invalid argument supplied to oneOfType, expected an instance of array.'); + + typeCheckPass(PropTypes.oneOf(PropTypes.string, PropTypes.number), []); }); it('should warn if none of the types are valid', function() {