From bef723e47f09f1136d6a94e4db81fb1ca59cb359 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 14 Feb 2017 14:53:50 -0800 Subject: [PATCH 1/9] Remove ReactPropTypeLocationNames module ReactPropTypeLocationNames was a map of location identifiers to location display names, for use in warnings. But in practice, this did nothing except rename "childContext" to "child context." Not worth it, IMO. Since we are going to add an API for running prop type checks manually, we need the ability to support arbitrary prop type locations. So each place that used to accept a ReactPropTypeLocation now just accepts a string. --- .../__tests__/ReactContextValidator-test.js | 4 +-- src/isomorphic/classic/class/ReactClass.js | 9 ++--- .../classic/types/ReactPropTypes.js | 33 +++++++------------ .../ReactPropTypesProduction-test.js | 4 +-- .../shared/fiber/ReactFiberContext.js | 2 +- .../reconciler/ReactCompositeComponent.js | 6 ++-- .../types/ReactPropTypeLocationNames.js | 29 ---------------- src/shared/types/ReactPropTypeLocations.js | 18 ---------- src/shared/types/checkReactTypeSpec.js | 9 ++--- 9 files changed, 23 insertions(+), 91 deletions(-) delete mode 100644 src/shared/types/ReactPropTypeLocationNames.js delete mode 100644 src/shared/types/ReactPropTypeLocations.js diff --git a/src/isomorphic/classic/__tests__/ReactContextValidator-test.js b/src/isomorphic/classic/__tests__/ReactContextValidator-test.js index 08582a200dd..a5a04430aae 100644 --- a/src/isomorphic/classic/__tests__/ReactContextValidator-test.js +++ b/src/isomorphic/classic/__tests__/ReactContextValidator-test.js @@ -261,7 +261,7 @@ describe('ReactContextValidator', () => { ReactTestUtils.renderIntoDocument(); expectDev(console.error.calls.count()).toBe(1); expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toBe( - 'Warning: Failed childContext type: ' + + 'Warning: Failed child context type: ' + 'The child context `foo` is marked as required in `Component`, but its ' + 'value is `undefined`.\n' + ' in Component (at **)' @@ -271,7 +271,7 @@ describe('ReactContextValidator', () => { expectDev(console.error.calls.count()).toBe(2); expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(1)[0])).toBe( - 'Warning: Failed childContext type: ' + + 'Warning: Failed child context type: ' + 'Invalid child context `foo` of type `number` ' + 'supplied to `Component`, expected `string`.\n' + ' in Component (at **)' diff --git a/src/isomorphic/classic/class/ReactClass.js b/src/isomorphic/classic/class/ReactClass.js index bf2f425f8f3..a4efdd825a0 100644 --- a/src/isomorphic/classic/class/ReactClass.js +++ b/src/isomorphic/classic/class/ReactClass.js @@ -13,7 +13,6 @@ var ReactBaseClasses = require('ReactBaseClasses'); var ReactElement = require('ReactElement'); -var ReactPropTypeLocationNames = require('ReactPropTypeLocationNames'); var ReactNoopUpdateQueue = require('ReactNoopUpdateQueue'); var emptyObject = require('emptyObject'); @@ -22,8 +21,6 @@ var warning = require('warning'); var ReactComponent = ReactBaseClasses.Component; -import type { ReactPropTypeLocations } from 'ReactPropTypeLocations'; - var MIXINS_KEY = 'mixins'; // Helper function to allow the creation of anonymous functions which do not @@ -330,7 +327,7 @@ var RESERVED_SPEC_KEYS = { validateTypeDef( Constructor, childContextTypes, - 'childContext' + 'child context' ); } Constructor.childContextTypes = Object.assign( @@ -390,7 +387,7 @@ var RESERVED_SPEC_KEYS = { function validateTypeDef( Constructor, typeDef, - location: ReactPropTypeLocations, + location: string, ) { for (var propName in typeDef) { if (typeDef.hasOwnProperty(propName)) { @@ -401,7 +398,7 @@ function validateTypeDef( '%s: %s type `%s` is invalid; it must be a function, usually from ' + 'React.PropTypes.', Constructor.displayName || 'ReactClass', - ReactPropTypeLocationNames[location], + location, propName ); } diff --git a/src/isomorphic/classic/types/ReactPropTypes.js b/src/isomorphic/classic/types/ReactPropTypes.js index dcd0dbc82a7..936de6dc759 100644 --- a/src/isomorphic/classic/types/ReactPropTypes.js +++ b/src/isomorphic/classic/types/ReactPropTypes.js @@ -12,7 +12,6 @@ 'use strict'; var ReactElement = require('ReactElement'); -var ReactPropTypeLocationNames = require('ReactPropTypeLocationNames'); var ReactPropTypesSecret = require('ReactPropTypesSecret'); var emptyFunction = require('emptyFunction'); @@ -192,16 +191,15 @@ function createChainableTypeChecker(validate) { } } if (props[propName] == null) { - var locationName = ReactPropTypeLocationNames[location]; if (isRequired) { if (props[propName] === null) { return new PropTypeError( - `The ${locationName} \`${propFullName}\` is marked as required ` + + `The ${location} \`${propFullName}\` is marked as required ` + `in \`${componentName}\`, but its value is \`null\`.` ); } return new PropTypeError( - `The ${locationName} \`${propFullName}\` is marked as required in ` + + `The ${location} \`${propFullName}\` is marked as required in ` + `\`${componentName}\`, but its value is \`undefined\`.` ); } @@ -235,14 +233,13 @@ function createPrimitiveTypeChecker(expectedType) { var propValue = props[propName]; var propType = getPropType(propValue); if (propType !== expectedType) { - var locationName = ReactPropTypeLocationNames[location]; // `propValue` being instance of, say, date/regexp, pass the 'object' // check, but we can offer a more precise error message here rather than // 'of type `object`'. var preciseType = getPreciseType(propValue); return new PropTypeError( - `Invalid ${locationName} \`${propFullName}\` of type ` + + `Invalid ${location} \`${propFullName}\` of type ` + `\`${preciseType}\` supplied to \`${componentName}\`, expected ` + `\`${expectedType}\`.` ); @@ -265,10 +262,9 @@ function createArrayOfTypeChecker(typeChecker) { } var propValue = props[propName]; if (!Array.isArray(propValue)) { - var locationName = ReactPropTypeLocationNames[location]; var propType = getPropType(propValue); return new PropTypeError( - `Invalid ${locationName} \`${propFullName}\` of type ` + + `Invalid ${location} \`${propFullName}\` of type ` + `\`${propType}\` supplied to \`${componentName}\`, expected an array.` ); } @@ -294,10 +290,9 @@ function createElementTypeChecker() { function validate(props, propName, componentName, location, propFullName) { var propValue = props[propName]; if (!ReactElement.isValidElement(propValue)) { - var locationName = ReactPropTypeLocationNames[location]; var propType = getPropType(propValue); return new PropTypeError( - `Invalid ${locationName} \`${propFullName}\` of type ` + + `Invalid ${location} \`${propFullName}\` of type ` + `\`${propType}\` supplied to \`${componentName}\`, expected a single ReactElement.` ); } @@ -309,11 +304,10 @@ function createElementTypeChecker() { function createInstanceTypeChecker(expectedClass) { function validate(props, propName, componentName, location, propFullName) { if (!(props[propName] instanceof expectedClass)) { - var locationName = ReactPropTypeLocationNames[location]; var expectedClassName = expectedClass.name || ANONYMOUS; var actualClassName = getClassName(props[propName]); return new PropTypeError( - `Invalid ${locationName} \`${propFullName}\` of type ` + + `Invalid ${location} \`${propFullName}\` of type ` + `\`${actualClassName}\` supplied to \`${componentName}\`, expected ` + `instance of \`${expectedClassName}\`.` ); @@ -337,10 +331,9 @@ function createEnumTypeChecker(expectedValues) { } } - var locationName = ReactPropTypeLocationNames[location]; var valuesString = JSON.stringify(expectedValues); return new PropTypeError( - `Invalid ${locationName} \`${propFullName}\` of value \`${propValue}\` ` + + `Invalid ${location} \`${propFullName}\` of value \`${propValue}\` ` + `supplied to \`${componentName}\`, expected one of ${valuesString}.` ); } @@ -357,9 +350,8 @@ function createObjectOfTypeChecker(typeChecker) { var propValue = props[propName]; var propType = getPropType(propValue); if (propType !== 'object') { - var locationName = ReactPropTypeLocationNames[location]; return new PropTypeError( - `Invalid ${locationName} \`${propFullName}\` of type ` + + `Invalid ${location} \`${propFullName}\` of type ` + `\`${propType}\` supplied to \`${componentName}\`, expected an object.` ); } @@ -406,9 +398,8 @@ function createUnionTypeChecker(arrayOfTypeCheckers) { } } - var locationName = ReactPropTypeLocationNames[location]; return new PropTypeError( - `Invalid ${locationName} \`${propFullName}\` supplied to ` + + `Invalid ${location} \`${propFullName}\` supplied to ` + `\`${componentName}\`.` ); } @@ -418,9 +409,8 @@ function createUnionTypeChecker(arrayOfTypeCheckers) { function createNodeChecker() { function validate(props, propName, componentName, location, propFullName) { if (!isNode(props[propName])) { - var locationName = ReactPropTypeLocationNames[location]; return new PropTypeError( - `Invalid ${locationName} \`${propFullName}\` supplied to ` + + `Invalid ${location} \`${propFullName}\` supplied to ` + `\`${componentName}\`, expected a ReactNode.` ); } @@ -434,9 +424,8 @@ function createShapeTypeChecker(shapeTypes) { var propValue = props[propName]; var propType = getPropType(propValue); if (propType !== 'object') { - var locationName = ReactPropTypeLocationNames[location]; return new PropTypeError( - `Invalid ${locationName} \`${propFullName}\` of type \`${propType}\` ` + + `Invalid ${location} \`${propFullName}\` of type \`${propType}\` ` + `supplied to \`${componentName}\`, expected \`object\`.` ); } diff --git a/src/isomorphic/classic/types/__tests__/ReactPropTypesProduction-test.js b/src/isomorphic/classic/types/__tests__/ReactPropTypesProduction-test.js index 076c1639349..8e6a868c9f9 100644 --- a/src/isomorphic/classic/types/__tests__/ReactPropTypesProduction-test.js +++ b/src/isomorphic/classic/types/__tests__/ReactPropTypesProduction-test.js @@ -14,7 +14,6 @@ describe('ReactPropTypesProduction', function() { var PropTypes; var React; - var ReactPropTypeLocations; var ReactTestUtils; var oldProcess; @@ -32,7 +31,6 @@ describe('ReactPropTypesProduction', function() { jest.resetModules(); PropTypes = require('ReactPropTypes'); React = require('React'); - ReactPropTypeLocations = require('ReactPropTypeLocations'); ReactTestUtils = require('ReactTestUtils'); }); @@ -48,7 +46,7 @@ describe('ReactPropTypesProduction', function() { props, 'testProp', 'testComponent', - ReactPropTypeLocations.prop + 'prop' ); }).toThrowError( 'React.PropTypes type checking code is stripped in production.' diff --git a/src/renderers/shared/fiber/ReactFiberContext.js b/src/renderers/shared/fiber/ReactFiberContext.js index 8b7383458b2..537c04fb4d8 100644 --- a/src/renderers/shared/fiber/ReactFiberContext.js +++ b/src/renderers/shared/fiber/ReactFiberContext.js @@ -182,7 +182,7 @@ function processChildContext(fiber : Fiber, parentContext : Object, isReconcilin // assume anything about the given fiber. We won't pass it down if we aren't sure. // TODO: remove this hack when we delete unstable_renderSubtree in Fiber. const workInProgress = isReconciling ? fiber : null; - checkReactTypeSpec(childContextTypes, childContext, 'childContext', name, null, workInProgress); + checkReactTypeSpec(childContextTypes, childContext, 'child context', name, null, workInProgress); } return {...parentContext, ...childContext}; } diff --git a/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js b/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js index 9a572a8f6f0..3bfb0ec08ed 100644 --- a/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js +++ b/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js @@ -33,8 +33,6 @@ var shallowEqual = require('shallowEqual'); var shouldUpdateReactComponent = require('shouldUpdateReactComponent'); var warning = require('warning'); -import type { ReactPropTypeLocations } from 'ReactPropTypeLocations'; - function StatelessComponent(Component) { } StatelessComponent.prototype.render = function() { @@ -687,7 +685,7 @@ var ReactCompositeComponent = { this._checkContextTypes( Component.childContextTypes, childContext, - 'childContext' + 'child context' ); } for (var name in childContext) { @@ -730,7 +728,7 @@ var ReactCompositeComponent = { _checkContextTypes: function( typeSpecs, values, - location: ReactPropTypeLocations, + location: string, ) { if (__DEV__) { checkReactTypeSpec( diff --git a/src/shared/types/ReactPropTypeLocationNames.js b/src/shared/types/ReactPropTypeLocationNames.js deleted file mode 100644 index 1b36b04cc89..00000000000 --- a/src/shared/types/ReactPropTypeLocationNames.js +++ /dev/null @@ -1,29 +0,0 @@ -/** - * Copyright 2013-present, Facebook, Inc. - * All rights reserved. - * - * This source code is licensed under the BSD-style license found in the - * LICENSE file in the root directory of this source tree. An additional grant - * of patent rights can be found in the PATENTS file in the same directory. - * - * @flow - * @providesModule ReactPropTypeLocationNames - */ - -'use strict'; - -import type { ReactPropTypeLocations } from 'ReactPropTypeLocations'; - -type NamesType = {[key: ReactPropTypeLocations]: string}; - -var ReactPropTypeLocationNames: NamesType = {}; - -if (__DEV__) { - ReactPropTypeLocationNames = { - prop: 'prop', - context: 'context', - childContext: 'child context', - }; -} - -module.exports = ReactPropTypeLocationNames; diff --git a/src/shared/types/ReactPropTypeLocations.js b/src/shared/types/ReactPropTypeLocations.js deleted file mode 100644 index a2cb905e191..00000000000 --- a/src/shared/types/ReactPropTypeLocations.js +++ /dev/null @@ -1,18 +0,0 @@ -/** - * Copyright 2013-present, Facebook, Inc. - * All rights reserved. - * - * This source code is licensed under the BSD-style license found in the - * LICENSE file in the root directory of this source tree. An additional grant - * of patent rights can be found in the PATENTS file in the same directory. - * - * @flow - * @providesModule ReactPropTypeLocations - */ - -'use strict'; - -export type ReactPropTypeLocations = - 'prop' | - 'context' | - 'childContext'; diff --git a/src/shared/types/checkReactTypeSpec.js b/src/shared/types/checkReactTypeSpec.js index e274c3e9f58..acb5032a666 100644 --- a/src/shared/types/checkReactTypeSpec.js +++ b/src/shared/types/checkReactTypeSpec.js @@ -11,14 +11,11 @@ 'use strict'; -var ReactPropTypeLocationNames = require('ReactPropTypeLocationNames'); var ReactPropTypesSecret = require('ReactPropTypesSecret'); var invariant = require('invariant'); var warning = require('warning'); -import type { ReactPropTypeLocations } from 'ReactPropTypeLocations'; - var ReactComponentTreeHook; if ( @@ -51,7 +48,7 @@ var loggedTypeFailures = {}; function checkReactTypeSpec( typeSpecs, values, - location: ReactPropTypeLocations, + location: string, componentName, element, // It is only safe to pass fiber if it is the work-in-progress version, and @@ -72,7 +69,7 @@ function checkReactTypeSpec( '%s: %s type `%s` is invalid; it must be a function, usually from ' + 'React.PropTypes.', componentName || 'React class', - ReactPropTypeLocationNames[location], + location, typeSpecName ); error = typeSpecs[typeSpecName](values, typeSpecName, componentName, location, null, ReactPropTypesSecret); @@ -87,7 +84,7 @@ function checkReactTypeSpec( 'creator (arrayOf, instanceOf, objectOf, oneOf, oneOfType, and ' + 'shape all require an argument).', componentName || 'React class', - ReactPropTypeLocationNames[location], + location, typeSpecName, typeof error ); From 600685bdefbb965129a73536a65b86cbaa73de48 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 14 Feb 2017 16:52:39 -0800 Subject: [PATCH 2/9] Add PropTypes.checkPropTypes API Allows users to check arbitrary objects against React prop types without relying on the implementation details of the prop validators themselves. An example of why this is important: some prop validators may throw, while others return an error object. Calling a prop validator manually requires handling both cases. `checkPropTypes` does this for you. --- .../classic/types/ReactPropTypes.js | 7 ++ .../types/__tests__/ReactPropTypes-test.js | 105 ++++++++---------- src/shared/types/checkReactTypeSpec.js | 4 +- 3 files changed, 58 insertions(+), 58 deletions(-) diff --git a/src/isomorphic/classic/types/ReactPropTypes.js b/src/isomorphic/classic/types/ReactPropTypes.js index 936de6dc759..ebbab9805b9 100644 --- a/src/isomorphic/classic/types/ReactPropTypes.js +++ b/src/isomorphic/classic/types/ReactPropTypes.js @@ -13,6 +13,7 @@ var ReactElement = require('ReactElement'); var ReactPropTypesSecret = require('ReactPropTypesSecret'); +var checkReactTypeSpec = require('checkReactTypeSpec'); var emptyFunction = require('emptyFunction'); var getIteratorFn = require('getIteratorFn'); @@ -122,6 +123,12 @@ if (__DEV__) { }; } +function checkPropTypes(propTypes, object, location, componentName, warnOnRepeat) { + checkReactTypeSpec(propTypes, object, location, componentName, null, null, warnOnRepeat); +} +ReactPropTypes.checkPropTypes = checkPropTypes; + + /** * inlined Object.is polyfill to avoid requiring consumers ship their own * https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/is diff --git a/src/isomorphic/classic/types/__tests__/ReactPropTypes-test.js b/src/isomorphic/classic/types/__tests__/ReactPropTypes-test.js index d482d3866df..824dffd8073 100644 --- a/src/isomorphic/classic/types/__tests__/ReactPropTypes-test.js +++ b/src/isomorphic/classic/types/__tests__/ReactPropTypes-test.js @@ -15,23 +15,36 @@ var PropTypes; var React; var ReactFragment; var ReactTestUtils; -var ReactPropTypesSecret; var Component; var MyComponent; -function typeCheckFail(declaration, value, message) { - var props = {testProp: value}; - var error = declaration( - props, - 'testProp', - 'testComponent', - 'prop', - null, - ReactPropTypesSecret - ); - expect(error instanceof Error).toBe(true); - expect(error.message).toBe(message); +function getPropTypeWarningMessage(propTypes, object, componentName) { + if (!console.error.calls) { + spyOn(console, 'error'); + } else { + console.error.calls.reset(); + } + PropTypes.checkPropTypes(propTypes, object, 'prop', 'testComponent', true); + const callCount = console.error.calls.count(); + if (callCount > 1) { + throw new Error('Too many warnings.'); + } + const message = console.error.calls.argsFor(0)[0] || null; + console.error.calls.reset(); + + return message; +} + +function typeCheckFail(declaration, value, expectedMessage) { + const propTypes = { + testProp: declaration, + }; + const props = { + testProp: value, + }; + const message = getPropTypeWarningMessage(propTypes, props, 'testComponent'); + expect(message).toContain(expectedMessage); } function typeCheckFailRequiredValues(declaration) { @@ -39,52 +52,31 @@ function typeCheckFailRequiredValues(declaration) { '`testComponent`, but its value is `null`.'; var unspecifiedMsg = 'The prop `testProp` is marked as required in ' + '`testComponent`, but its value is \`undefined\`.'; - var props1 = {testProp: null}; - var error1 = declaration( - props1, - 'testProp', - 'testComponent', - 'prop', - null, - ReactPropTypesSecret - ); - expect(error1 instanceof Error).toBe(true); - expect(error1.message).toBe(specifiedButIsNullMsg); - var props2 = {testProp: undefined}; - var error2 = declaration( - props2, - 'testProp', - 'testComponent', - 'prop', - null, - ReactPropTypesSecret - ); - expect(error2 instanceof Error).toBe(true); - expect(error2.message).toBe(unspecifiedMsg); - var props3 = {}; - var error3 = declaration( - props3, - 'testProp', - 'testComponent', - 'prop', - null, - ReactPropTypesSecret - ); - expect(error3 instanceof Error).toBe(true); - expect(error3.message).toBe(unspecifiedMsg); + + var propTypes = { testProp: declaration }; + + // Required prop is null + var message1 = getPropTypeWarningMessage(propTypes, { testProp: null }, 'testComponent'); + expect(message1).toContain(specifiedButIsNullMsg); + + // Required prop is undefined + var message2 = getPropTypeWarningMessage(propTypes, { testProp: undefined }, 'testComponent'); + expect(message2).toContain(unspecifiedMsg); + + // Required prop is not a member of props object + var message3 = getPropTypeWarningMessage(propTypes, {}, 'testComponent'); + expect(message3).toContain(unspecifiedMsg); } function typeCheckPass(declaration, value) { - var props = {testProp: value}; - var error = declaration( - props, - 'testProp', - 'testComponent', - 'prop', - null, - ReactPropTypesSecret - ); - expect(error).toBe(null); + const propTypes = { + testProp: declaration, + }; + const props = { + testProp: value, + }; + const message = getPropTypeWarningMessage(propTypes, props, 'testComponent'); + expect(message).toBe(null); } function expectWarningInDevelopment(declaration, value) { @@ -112,7 +104,6 @@ describe('ReactPropTypes', () => { React = require('React'); ReactFragment = require('ReactFragment'); ReactTestUtils = require('ReactTestUtils'); - ReactPropTypesSecret = require('ReactPropTypesSecret'); }); describe('Primitive Types', () => { diff --git a/src/shared/types/checkReactTypeSpec.js b/src/shared/types/checkReactTypeSpec.js index acb5032a666..e31114276cb 100644 --- a/src/shared/types/checkReactTypeSpec.js +++ b/src/shared/types/checkReactTypeSpec.js @@ -54,6 +54,8 @@ function checkReactTypeSpec( // It is only safe to pass fiber if it is the work-in-progress version, and // only during reconciliation (begin and complete phase). workInProgressOrDebugID, + // Default behavior is to suppress repeated warnings, but can be overridden + warnOnRepeat ) { for (var typeSpecName in typeSpecs) { if (typeSpecs.hasOwnProperty(typeSpecName)) { @@ -88,7 +90,7 @@ function checkReactTypeSpec( typeSpecName, typeof error ); - if (error instanceof Error && !(error.message in loggedTypeFailures)) { + if (error instanceof Error && (warnOnRepeat || !(error.message in loggedTypeFailures))) { // Only monitor this failure once because there tends to be a lot of the // same error. loggedTypeFailures[error.message] = true; From e4acd12a9b995410188c6a551b069c6583a88cca Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 15 Feb 2017 14:10:14 -0800 Subject: [PATCH 3/9] Test that checkPropTypes does not throw or return a value --- scripts/fiber/tests-passing.txt | 2 ++ .../types/__tests__/ReactPropTypes-test.js | 28 +++++++++++++++++++ 2 files changed, 30 insertions(+) diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 5152468f42d..b24e98e993e 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -307,6 +307,8 @@ src/isomorphic/classic/element/__tests__/ReactElementValidator-test.js * does not blow up on key warning with undefined type src/isomorphic/classic/types/__tests__/ReactPropTypes-test.js +* does not return a value from a validator +* does not throw if validator throws * should warn for invalid strings * should fail date and regexp correctly * should not warn for valid values diff --git a/src/isomorphic/classic/types/__tests__/ReactPropTypes-test.js b/src/isomorphic/classic/types/__tests__/ReactPropTypes-test.js index 824dffd8073..83c6f46baa2 100644 --- a/src/isomorphic/classic/types/__tests__/ReactPropTypes-test.js +++ b/src/isomorphic/classic/types/__tests__/ReactPropTypes-test.js @@ -106,6 +106,34 @@ describe('ReactPropTypes', () => { ReactTestUtils = require('ReactTestUtils'); }); + describe('checkPropTypes', () => { + it('does not return a value from a validator', () => { + spyOn(console, 'error'); + const propTypes = { + foo(props, propName, componentName) { + return new Error('some error'); + }, + }; + const props = { foo: 'foo' }; + const returnValue = PropTypes.checkPropTypes(propTypes, props, 'prop', 'testComponent', true); + expect(console.error.calls.argsFor(0)[0]).toContain('some error'); + expect(returnValue).toBe(undefined); + }); + + it('does not throw if validator throws', () => { + spyOn(console, 'error'); + const propTypes = { + foo(props, propName, componentName) { + throw new Error('some error'); + }, + }; + const props = { foo: 'foo' }; + const returnValue = PropTypes.checkPropTypes(propTypes, props, 'prop', 'testComponent', true); + expect(console.error.calls.argsFor(0)[0]).toContain('some error'); + expect(returnValue).toBe(undefined); + }); + }); + describe('Primitive Types', () => { it('should warn for invalid strings', () => { typeCheckFail( From 3746a9dafb65e077632a79870a75120aa16c4c7f Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 15 Feb 2017 16:08:15 -0800 Subject: [PATCH 4/9] Invert dependency between checkPropTypes and checkReactTypeSpec Expressing checkReactTypeSpec using the public API frees us to move it to a separate module in the future. --- .../classic/types/ReactPropTypes.js | 5 +- .../types/__tests__/ReactPropTypes-test.js | 8 +- .../classic/types/checkPropTypes.js | 85 ++++++++++++++ src/shared/types/checkReactTypeSpec.js | 108 ++++-------------- 4 files changed, 114 insertions(+), 92 deletions(-) create mode 100644 src/isomorphic/classic/types/checkPropTypes.js diff --git a/src/isomorphic/classic/types/ReactPropTypes.js b/src/isomorphic/classic/types/ReactPropTypes.js index ebbab9805b9..0225942cdd2 100644 --- a/src/isomorphic/classic/types/ReactPropTypes.js +++ b/src/isomorphic/classic/types/ReactPropTypes.js @@ -13,7 +13,7 @@ var ReactElement = require('ReactElement'); var ReactPropTypesSecret = require('ReactPropTypesSecret'); -var checkReactTypeSpec = require('checkReactTypeSpec'); +var checkPropTypes = require('checkPropTypes'); var emptyFunction = require('emptyFunction'); var getIteratorFn = require('getIteratorFn'); @@ -123,9 +123,6 @@ if (__DEV__) { }; } -function checkPropTypes(propTypes, object, location, componentName, warnOnRepeat) { - checkReactTypeSpec(propTypes, object, location, componentName, null, null, warnOnRepeat); -} ReactPropTypes.checkPropTypes = checkPropTypes; diff --git a/src/isomorphic/classic/types/__tests__/ReactPropTypes-test.js b/src/isomorphic/classic/types/__tests__/ReactPropTypes-test.js index 83c6f46baa2..6a1aba0b8a1 100644 --- a/src/isomorphic/classic/types/__tests__/ReactPropTypes-test.js +++ b/src/isomorphic/classic/types/__tests__/ReactPropTypes-test.js @@ -12,6 +12,7 @@ 'use strict'; var PropTypes; +var checkReactTypeSpec; var React; var ReactFragment; var ReactTestUtils; @@ -25,7 +26,7 @@ function getPropTypeWarningMessage(propTypes, object, componentName) { } else { console.error.calls.reset(); } - PropTypes.checkPropTypes(propTypes, object, 'prop', 'testComponent', true); + checkReactTypeSpec(propTypes, object, 'prop', 'testComponent', null, null, true); const callCount = console.error.calls.count(); if (callCount > 1) { throw new Error('Too many warnings.'); @@ -101,6 +102,7 @@ function expectWarningInDevelopment(declaration, value) { describe('ReactPropTypes', () => { beforeEach(() => { PropTypes = require('ReactPropTypes'); + checkReactTypeSpec = require('checkReactTypeSpec'); React = require('React'); ReactFragment = require('ReactFragment'); ReactTestUtils = require('ReactTestUtils'); @@ -115,7 +117,7 @@ describe('ReactPropTypes', () => { }, }; const props = { foo: 'foo' }; - const returnValue = PropTypes.checkPropTypes(propTypes, props, 'prop', 'testComponent', true); + const returnValue = PropTypes.checkPropTypes(propTypes, props, 'prop', 'testComponent', null, true); expect(console.error.calls.argsFor(0)[0]).toContain('some error'); expect(returnValue).toBe(undefined); }); @@ -128,7 +130,7 @@ describe('ReactPropTypes', () => { }, }; const props = { foo: 'foo' }; - const returnValue = PropTypes.checkPropTypes(propTypes, props, 'prop', 'testComponent', true); + const returnValue = PropTypes.checkPropTypes(propTypes, props, 'prop', 'testComponent', null, true); expect(console.error.calls.argsFor(0)[0]).toContain('some error'); expect(returnValue).toBe(undefined); }); diff --git a/src/isomorphic/classic/types/checkPropTypes.js b/src/isomorphic/classic/types/checkPropTypes.js new file mode 100644 index 00000000000..d38468f38b5 --- /dev/null +++ b/src/isomorphic/classic/types/checkPropTypes.js @@ -0,0 +1,85 @@ +/** + * Copyright 2013-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + * @providesModule checkPropTypes + */ + +'use strict'; + +var ReactPropTypesSecret = require('ReactPropTypesSecret'); + +var invariant = require('invariant'); +var warning = require('warning'); + +var loggedTypeFailures = {}; + +/** + * Assert that the values match with the type specs. + * Error messages are memorized and will only be shown once. + * + * @param {object} typeSpecs Map of name to a ReactPropType + * @param {object} values Runtime values that need to be type-checked + * @param {string} location e.g. "prop", "context", "child context" + * @param {string} componentName Name of the component for error messages. + * @param {?Function} formatMessage Function that transforms the error message, to add additional info. + * @param {?boolean} warnOnRepeat Whether or not repeated warnings should be skipped. + * @private + */ +function checkPropTypes(typeSpecs, values, location, componentName, formatMessage, warnOnRepeat) { + for (var typeSpecName in typeSpecs) { + if (typeSpecs.hasOwnProperty(typeSpecName)) { + var error; + // Prop type validation may throw. In case they do, we don't want to + // fail the render phase where it didn't fail before. So we log it. + // After these have been cleaned up, we'll let them throw. + try { + // This is intentionally an invariant that gets caught. It's the same + // behavior as without this statement except with a better message. + invariant( + typeof typeSpecs[typeSpecName] === 'function', + '%s: %s type `%s` is invalid; it must be a function, usually from ' + + 'React.PropTypes.', + componentName || 'React class', + location, + typeSpecName + ); + error = typeSpecs[typeSpecName](values, typeSpecName, componentName, location, null, ReactPropTypesSecret); + } catch (ex) { + error = ex; + } + warning( + !error || error instanceof Error, + '%s: type specification of %s `%s` is invalid; the type checker ' + + 'function must return `null` or an `Error` but returned a %s. ' + + 'You may have forgotten to pass an argument to the type checker ' + + 'creator (arrayOf, instanceOf, objectOf, oneOf, oneOfType, and ' + + 'shape all require an argument).', + componentName || 'React class', + location, + typeSpecName, + typeof error + ); + if (error instanceof Error && (warnOnRepeat || !(error.message in loggedTypeFailures))) { + // Only monitor this failure once because there tends to be a lot of the + // same error. + loggedTypeFailures[error.message] = true; + + var message = formatMessage ? formatMessage(error.message) : error.message; + + warning( + false, + 'Failed %s type: %s', + location, + message, + ); + } + } + } +} + +module.exports = checkPropTypes; diff --git a/src/shared/types/checkReactTypeSpec.js b/src/shared/types/checkReactTypeSpec.js index e31114276cb..819ab91a9aa 100644 --- a/src/shared/types/checkReactTypeSpec.js +++ b/src/shared/types/checkReactTypeSpec.js @@ -11,10 +11,7 @@ 'use strict'; -var ReactPropTypesSecret = require('ReactPropTypesSecret'); - -var invariant = require('invariant'); -var warning = require('warning'); +var checkPropTypes = require('checkPropTypes'); var ReactComponentTreeHook; @@ -31,20 +28,6 @@ if ( ReactComponentTreeHook = require('ReactComponentTreeHook'); } -var loggedTypeFailures = {}; - -/** - * Assert that the values match with the type specs. - * Error messages are memorized and will only be shown once. - * - * @param {object} typeSpecs Map of name to a ReactPropType - * @param {object} values Runtime values that need to be type-checked - * @param {string} location e.g. "prop", "context", "child context" - * @param {string} componentName Name of the component for error messages. - * @param {?object} element The React element that is being type-checked - * @param {?number} workInProgressOrDebugID The React component instance that is being type-checked - * @private - */ function checkReactTypeSpec( typeSpecs, values, @@ -54,80 +37,35 @@ function checkReactTypeSpec( // It is only safe to pass fiber if it is the work-in-progress version, and // only during reconciliation (begin and complete phase). workInProgressOrDebugID, - // Default behavior is to suppress repeated warnings, but can be overridden warnOnRepeat ) { - for (var typeSpecName in typeSpecs) { - if (typeSpecs.hasOwnProperty(typeSpecName)) { - var error; - // Prop type validation may throw. In case they do, we don't want to - // fail the render phase where it didn't fail before. So we log it. - // After these have been cleaned up, we'll let them throw. - try { - // This is intentionally an invariant that gets caught. It's the same - // behavior as without this statement except with a better message. - invariant( - typeof typeSpecs[typeSpecName] === 'function', - '%s: %s type `%s` is invalid; it must be a function, usually from ' + - 'React.PropTypes.', - componentName || 'React class', - location, - typeSpecName - ); - error = typeSpecs[typeSpecName](values, typeSpecName, componentName, location, null, ReactPropTypesSecret); - } catch (ex) { - error = ex; + function formatMessage(message) { + if (__DEV__) { + let componentStackInfo = ''; + if (!ReactComponentTreeHook) { + ReactComponentTreeHook = require('ReactComponentTreeHook'); } - warning( - !error || error instanceof Error, - '%s: type specification of %s `%s` is invalid; the type checker ' + - 'function must return `null` or an `Error` but returned a %s. ' + - 'You may have forgotten to pass an argument to the type checker ' + - 'creator (arrayOf, instanceOf, objectOf, oneOf, oneOfType, and ' + - 'shape all require an argument).', - componentName || 'React class', - location, - typeSpecName, - typeof error - ); - if (error instanceof Error && (warnOnRepeat || !(error.message in loggedTypeFailures))) { - // Only monitor this failure once because there tends to be a lot of the - // same error. - loggedTypeFailures[error.message] = true; - - var componentStackInfo = ''; - - if (__DEV__) { - if (!ReactComponentTreeHook) { - ReactComponentTreeHook = require('ReactComponentTreeHook'); - } - if (workInProgressOrDebugID != null) { - if (typeof workInProgressOrDebugID === 'number') { - // DebugID from Stack. - const debugID = workInProgressOrDebugID; - componentStackInfo = ReactComponentTreeHook.getStackAddendumByID(debugID); - } else if (typeof workInProgressOrDebugID.tag === 'number') { - // This is a Fiber. - // The stack will only be correct if this is a work in progress - // version and we're calling it during reconciliation. - const workInProgress = workInProgressOrDebugID; - componentStackInfo = ReactComponentTreeHook.getStackAddendumByWorkInProgressFiber(workInProgress); - } - } else if (element !== null) { - componentStackInfo = ReactComponentTreeHook.getCurrentStackAddendum(element); - } + if (workInProgressOrDebugID != null) { + if (typeof workInProgressOrDebugID === 'number') { + // DebugID from Stack. + const debugID = workInProgressOrDebugID; + componentStackInfo = ReactComponentTreeHook.getStackAddendumByID(debugID); + } else if (typeof workInProgressOrDebugID.tag === 'number') { + // This is a Fiber. + // The stack will only be correct if this is a work in progress + // version and we're calling it during reconciliation. + const workInProgress = workInProgressOrDebugID; + componentStackInfo = ReactComponentTreeHook.getStackAddendumByWorkInProgressFiber(workInProgress); } - - warning( - false, - 'Failed %s type: %s%s', - location, - error.message, - componentStackInfo - ); + } else if (element !== null) { + componentStackInfo = ReactComponentTreeHook.getCurrentStackAddendum(element); } + message += componentStackInfo; } + return message; } + + checkPropTypes(typeSpecs, values, location, componentName, formatMessage, warnOnRepeat); } module.exports = checkReactTypeSpec; From 6ff5211dee0c1734b20d5ce4cd0a9e665f043d72 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Thu, 16 Feb 2017 11:28:20 -0800 Subject: [PATCH 5/9] Remove warnOnRepeat option Don't need this internally; doubt external users will need it either --- .../classic/types/__tests__/ReactPropTypes-test.js | 14 ++++++++++---- src/isomorphic/classic/types/checkPropTypes.js | 5 ++--- src/shared/types/checkReactTypeSpec.js | 5 ++--- 3 files changed, 14 insertions(+), 10 deletions(-) diff --git a/src/isomorphic/classic/types/__tests__/ReactPropTypes-test.js b/src/isomorphic/classic/types/__tests__/ReactPropTypes-test.js index 6a1aba0b8a1..0653fae8a7c 100644 --- a/src/isomorphic/classic/types/__tests__/ReactPropTypes-test.js +++ b/src/isomorphic/classic/types/__tests__/ReactPropTypes-test.js @@ -20,13 +20,19 @@ var ReactTestUtils; var Component; var MyComponent; +function resetWarningCache() { + jest.resetModules(); + checkReactTypeSpec = require('checkReactTypeSpec'); +} + function getPropTypeWarningMessage(propTypes, object, componentName) { if (!console.error.calls) { spyOn(console, 'error'); } else { console.error.calls.reset(); } - checkReactTypeSpec(propTypes, object, 'prop', 'testComponent', null, null, true); + resetWarningCache(); + checkReactTypeSpec(propTypes, object, 'prop', 'testComponent', null, null); const callCount = console.error.calls.count(); if (callCount > 1) { throw new Error('Too many warnings.'); @@ -102,10 +108,10 @@ function expectWarningInDevelopment(declaration, value) { describe('ReactPropTypes', () => { beforeEach(() => { PropTypes = require('ReactPropTypes'); - checkReactTypeSpec = require('checkReactTypeSpec'); React = require('React'); ReactFragment = require('ReactFragment'); ReactTestUtils = require('ReactTestUtils'); + resetWarningCache(); }); describe('checkPropTypes', () => { @@ -117,7 +123,7 @@ describe('ReactPropTypes', () => { }, }; const props = { foo: 'foo' }; - const returnValue = PropTypes.checkPropTypes(propTypes, props, 'prop', 'testComponent', null, true); + const returnValue = PropTypes.checkPropTypes(propTypes, props, 'prop', 'testComponent', null); expect(console.error.calls.argsFor(0)[0]).toContain('some error'); expect(returnValue).toBe(undefined); }); @@ -130,7 +136,7 @@ describe('ReactPropTypes', () => { }, }; const props = { foo: 'foo' }; - const returnValue = PropTypes.checkPropTypes(propTypes, props, 'prop', 'testComponent', null, true); + const returnValue = PropTypes.checkPropTypes(propTypes, props, 'prop', 'testComponent', null); expect(console.error.calls.argsFor(0)[0]).toContain('some error'); expect(returnValue).toBe(undefined); }); diff --git a/src/isomorphic/classic/types/checkPropTypes.js b/src/isomorphic/classic/types/checkPropTypes.js index d38468f38b5..b0d53a1ac73 100644 --- a/src/isomorphic/classic/types/checkPropTypes.js +++ b/src/isomorphic/classic/types/checkPropTypes.js @@ -27,10 +27,9 @@ var loggedTypeFailures = {}; * @param {string} location e.g. "prop", "context", "child context" * @param {string} componentName Name of the component for error messages. * @param {?Function} formatMessage Function that transforms the error message, to add additional info. - * @param {?boolean} warnOnRepeat Whether or not repeated warnings should be skipped. * @private */ -function checkPropTypes(typeSpecs, values, location, componentName, formatMessage, warnOnRepeat) { +function checkPropTypes(typeSpecs, values, location, componentName, formatMessage) { for (var typeSpecName in typeSpecs) { if (typeSpecs.hasOwnProperty(typeSpecName)) { var error; @@ -64,7 +63,7 @@ function checkPropTypes(typeSpecs, values, location, componentName, formatMessag typeSpecName, typeof error ); - if (error instanceof Error && (warnOnRepeat || !(error.message in loggedTypeFailures))) { + if (error instanceof Error && !(error.message in loggedTypeFailures)) { // Only monitor this failure once because there tends to be a lot of the // same error. loggedTypeFailures[error.message] = true; diff --git a/src/shared/types/checkReactTypeSpec.js b/src/shared/types/checkReactTypeSpec.js index 819ab91a9aa..663a18274a6 100644 --- a/src/shared/types/checkReactTypeSpec.js +++ b/src/shared/types/checkReactTypeSpec.js @@ -36,8 +36,7 @@ function checkReactTypeSpec( element, // It is only safe to pass fiber if it is the work-in-progress version, and // only during reconciliation (begin and complete phase). - workInProgressOrDebugID, - warnOnRepeat + workInProgressOrDebugID ) { function formatMessage(message) { if (__DEV__) { @@ -65,7 +64,7 @@ function checkReactTypeSpec( return message; } - checkPropTypes(typeSpecs, values, location, componentName, formatMessage, warnOnRepeat); + checkPropTypes(typeSpecs, values, location, componentName, formatMessage); } module.exports = checkReactTypeSpec; From 9993fdd957248beb8950cb3a6986ddcff7437ea0 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Thu, 16 Feb 2017 11:34:49 -0800 Subject: [PATCH 6/9] Change formatMessage parameter to getStack Gives us flexibility in the future to deal with the stack and the message separately. --- src/isomorphic/classic/types/checkPropTypes.js | 11 ++++++----- src/shared/types/checkReactTypeSpec.js | 15 +++++++-------- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/src/isomorphic/classic/types/checkPropTypes.js b/src/isomorphic/classic/types/checkPropTypes.js index b0d53a1ac73..20961aca86a 100644 --- a/src/isomorphic/classic/types/checkPropTypes.js +++ b/src/isomorphic/classic/types/checkPropTypes.js @@ -26,10 +26,10 @@ var loggedTypeFailures = {}; * @param {object} values Runtime values that need to be type-checked * @param {string} location e.g. "prop", "context", "child context" * @param {string} componentName Name of the component for error messages. - * @param {?Function} formatMessage Function that transforms the error message, to add additional info. + * @param {?Function} getStack Returns the component stack. * @private */ -function checkPropTypes(typeSpecs, values, location, componentName, formatMessage) { +function checkPropTypes(typeSpecs, values, location, componentName, getStack) { for (var typeSpecName in typeSpecs) { if (typeSpecs.hasOwnProperty(typeSpecName)) { var error; @@ -68,13 +68,14 @@ function checkPropTypes(typeSpecs, values, location, componentName, formatMessag // same error. loggedTypeFailures[error.message] = true; - var message = formatMessage ? formatMessage(error.message) : error.message; + var stack = getStack ? getStack() : ''; warning( false, - 'Failed %s type: %s', + 'Failed %s type: %s%s', location, - message, + error.message, + stack, ); } } diff --git a/src/shared/types/checkReactTypeSpec.js b/src/shared/types/checkReactTypeSpec.js index 663a18274a6..2f617efa918 100644 --- a/src/shared/types/checkReactTypeSpec.js +++ b/src/shared/types/checkReactTypeSpec.js @@ -38,9 +38,9 @@ function checkReactTypeSpec( // only during reconciliation (begin and complete phase). workInProgressOrDebugID ) { - function formatMessage(message) { + function getStack() { + let stack = ''; if (__DEV__) { - let componentStackInfo = ''; if (!ReactComponentTreeHook) { ReactComponentTreeHook = require('ReactComponentTreeHook'); } @@ -48,23 +48,22 @@ function checkReactTypeSpec( if (typeof workInProgressOrDebugID === 'number') { // DebugID from Stack. const debugID = workInProgressOrDebugID; - componentStackInfo = ReactComponentTreeHook.getStackAddendumByID(debugID); + stack = ReactComponentTreeHook.getStackAddendumByID(debugID); } else if (typeof workInProgressOrDebugID.tag === 'number') { // This is a Fiber. // The stack will only be correct if this is a work in progress // version and we're calling it during reconciliation. const workInProgress = workInProgressOrDebugID; - componentStackInfo = ReactComponentTreeHook.getStackAddendumByWorkInProgressFiber(workInProgress); + stack = ReactComponentTreeHook.getStackAddendumByWorkInProgressFiber(workInProgress); } } else if (element !== null) { - componentStackInfo = ReactComponentTreeHook.getCurrentStackAddendum(element); + stack = ReactComponentTreeHook.getCurrentStackAddendum(element); } - message += componentStackInfo; } - return message; + return stack; } - checkPropTypes(typeSpecs, values, location, componentName, formatMessage); + checkPropTypes(typeSpecs, values, location, componentName, getStack); } module.exports = checkReactTypeSpec; From e6d6ad3e88b0728b917969091dede0ff14bf5e43 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 21 Feb 2017 13:53:43 -0800 Subject: [PATCH 7/9] Remove closure in checkReactTypeSpec by tracking stack info globally There's a lot of overlap between ReactCurrentOwner, ReactDebugCurrentFrame, and ReactDebugCurrentFiber that should be consolidated. --- .../classic/element/ReactDebugCurrentFrame.js | 58 +++++++++++++++++++ .../classic/element/ReactElementValidator.js | 24 ++++++-- .../types/__tests__/ReactPropTypes-test.js | 2 +- .../classic/types/checkPropTypes.js | 2 +- .../shared/fiber/ReactFiberContext.js | 9 ++- .../reconciler/ReactCompositeComponent.js | 7 ++- src/shared/types/checkReactTypeSpec.js | 54 +++-------------- 7 files changed, 100 insertions(+), 56 deletions(-) create mode 100644 src/isomorphic/classic/element/ReactDebugCurrentFrame.js diff --git a/src/isomorphic/classic/element/ReactDebugCurrentFrame.js b/src/isomorphic/classic/element/ReactDebugCurrentFrame.js new file mode 100644 index 00000000000..f2e4676aeda --- /dev/null +++ b/src/isomorphic/classic/element/ReactDebugCurrentFrame.js @@ -0,0 +1,58 @@ +/** + * Copyright 2013-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + * @providesModule ReactDebugCurrentFrame + * @flow + */ + +'use strict'; + +import type { Fiber } from 'ReactFiber'; +import type { DebugID } from 'ReactInstanceType'; + +if (__DEV__) { + var { + getStackAddendumByID, + getStackAddendumByWorkInProgressFiber, + getCurrentStackAddendum, + } = require('ReactComponentTreeHook'); +} + +const ReactDebugCurrentFrame = { + // Component that is being worked on + current: (null : Fiber | DebugID | null), + + // Element that is being cloned or created + element: (null : *), + + getStackAddendum() : string | null { + let stack = null; + if (__DEV__) { + const current = ReactDebugCurrentFrame.current; + const element = ReactDebugCurrentFrame.element; + if (current !== null) { + if (typeof current === 'number') { + // DebugID from Stack. + const debugID = current; + stack = getStackAddendumByID(debugID); + } else if (typeof current.tag === 'number') { + // This is a Fiber. + // The stack will only be correct if this is a work in progress + // version and we're calling it during reconciliation. + const workInProgress = current; + stack = getStackAddendumByWorkInProgressFiber(workInProgress); + } + } else if (element !== null) { + stack = getCurrentStackAddendum(element); + } + } + return stack; + }, +}; + +module.exports = ReactDebugCurrentFrame; diff --git a/src/isomorphic/classic/element/ReactElementValidator.js b/src/isomorphic/classic/element/ReactElementValidator.js index b8146bfd86d..224ad49644a 100644 --- a/src/isomorphic/classic/element/ReactElementValidator.js +++ b/src/isomorphic/classic/element/ReactElementValidator.js @@ -27,7 +27,11 @@ var checkReactTypeSpec = require('checkReactTypeSpec'); var canDefineProperty = require('canDefineProperty'); var getComponentName = require('getComponentName'); var getIteratorFn = require('getIteratorFn'); -var warning = require('warning'); + +if (__DEV__) { + var warning = require('warning'); + var ReactDebugCurrentFrame = require('ReactDebugCurrentFrame'); +} function getDeclarationErrorAddendum() { if (ReactCurrentOwner.current) { @@ -181,9 +185,7 @@ function validatePropTypes(element) { componentClass.propTypes, element.props, 'prop', - name, - element, - null + name ); } if (typeof componentClass.getDefaultProps === 'function') { @@ -248,6 +250,10 @@ var ReactElementValidator = { return element; } + if (__DEV__) { + ReactDebugCurrentFrame.element = element; + } + // Skip key warning if the type isn't valid since our key validation logic // doesn't expect a non-string/function type and can throw confusing errors. // We don't want exception behavior to differ between dev and prod. @@ -261,6 +267,10 @@ var ReactElementValidator = { validatePropTypes(element); + if (__DEV__) { + ReactDebugCurrentFrame.element = null; + } + return element; }, @@ -301,10 +311,16 @@ var ReactElementValidator = { cloneElement: function(element, props, children) { var newElement = ReactElement.cloneElement.apply(this, arguments); + if (__DEV__) { + ReactDebugCurrentFrame.element = newElement; + } for (var i = 2; i < arguments.length; i++) { validateChildKeys(arguments[i], newElement.type); } validatePropTypes(newElement); + if (__DEV__) { + ReactDebugCurrentFrame.element = null; + } return newElement; }, diff --git a/src/isomorphic/classic/types/__tests__/ReactPropTypes-test.js b/src/isomorphic/classic/types/__tests__/ReactPropTypes-test.js index 0653fae8a7c..b8f6aefb960 100644 --- a/src/isomorphic/classic/types/__tests__/ReactPropTypes-test.js +++ b/src/isomorphic/classic/types/__tests__/ReactPropTypes-test.js @@ -32,7 +32,7 @@ function getPropTypeWarningMessage(propTypes, object, componentName) { console.error.calls.reset(); } resetWarningCache(); - checkReactTypeSpec(propTypes, object, 'prop', 'testComponent', null, null); + checkReactTypeSpec(propTypes, object, 'prop', 'testComponent'); const callCount = console.error.calls.count(); if (callCount > 1) { throw new Error('Too many warnings.'); diff --git a/src/isomorphic/classic/types/checkPropTypes.js b/src/isomorphic/classic/types/checkPropTypes.js index 20961aca86a..172b96842e4 100644 --- a/src/isomorphic/classic/types/checkPropTypes.js +++ b/src/isomorphic/classic/types/checkPropTypes.js @@ -75,7 +75,7 @@ function checkPropTypes(typeSpecs, values, location, componentName, getStack) { 'Failed %s type: %s%s', location, error.message, - stack, + stack != null ? stack : '', ); } } diff --git a/src/renderers/shared/fiber/ReactFiberContext.js b/src/renderers/shared/fiber/ReactFiberContext.js index 537c04fb4d8..836bc79eb42 100644 --- a/src/renderers/shared/fiber/ReactFiberContext.js +++ b/src/renderers/shared/fiber/ReactFiberContext.js @@ -34,6 +34,7 @@ const { if (__DEV__) { var checkReactTypeSpec = require('checkReactTypeSpec'); + var ReactDebugCurrentFrame = require('ReactDebugCurrentFrame'); var warnedAboutMissingGetChildContext = {}; } @@ -91,7 +92,9 @@ exports.getMaskedContext = function(workInProgress : Fiber, unmaskedContext : Ob if (__DEV__) { const name = getComponentName(workInProgress); - checkReactTypeSpec(contextTypes, context, 'context', name, null, workInProgress); + ReactDebugCurrentFrame.current = workInProgress; + checkReactTypeSpec(contextTypes, context, 'context', name); + ReactDebugCurrentFrame.current = null; } // Cache unmasked context so we can avoid recreating masked context unless necessary. @@ -182,7 +185,9 @@ function processChildContext(fiber : Fiber, parentContext : Object, isReconcilin // assume anything about the given fiber. We won't pass it down if we aren't sure. // TODO: remove this hack when we delete unstable_renderSubtree in Fiber. const workInProgress = isReconciling ? fiber : null; - checkReactTypeSpec(childContextTypes, childContext, 'child context', name, null, workInProgress); + ReactDebugCurrentFrame.current = workInProgress; + checkReactTypeSpec(childContextTypes, childContext, 'child context', name); + ReactDebugCurrentFrame.current = null; } return {...parentContext, ...childContext}; } diff --git a/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js b/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js index 3bfb0ec08ed..19ce8079f2c 100644 --- a/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js +++ b/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js @@ -24,6 +24,7 @@ var ReactReconciler = require('ReactReconciler'); if (__DEV__) { var checkReactTypeSpec = require('checkReactTypeSpec'); + var ReactDebugCurrentFrame = require('ReactDebugCurrentFrame'); var warningAboutMissingGetChildContext = {}; } @@ -731,14 +732,14 @@ var ReactCompositeComponent = { location: string, ) { if (__DEV__) { + ReactDebugCurrentFrame.current = this._debugID; checkReactTypeSpec( typeSpecs, values, location, - this.getName(), - null, - this._debugID + this.getName() ); + ReactDebugCurrentFrame.current = null; } }, diff --git a/src/shared/types/checkReactTypeSpec.js b/src/shared/types/checkReactTypeSpec.js index 2f617efa918..f52fe68e477 100644 --- a/src/shared/types/checkReactTypeSpec.js +++ b/src/shared/types/checkReactTypeSpec.js @@ -13,57 +13,21 @@ var checkPropTypes = require('checkPropTypes'); -var ReactComponentTreeHook; - -if ( - typeof process !== 'undefined' && - process.env && - process.env.NODE_ENV === 'test' -) { - // Temporary hack. - // Inline requires don't work well with Jest: - // https://github.com/facebook/react/issues/7240 - // Remove the inline requires when we don't need them anymore: - // https://github.com/facebook/react/pull/7178 - ReactComponentTreeHook = require('ReactComponentTreeHook'); -} +var { getStackAddendum } = require('ReactDebugCurrentFrame'); function checkReactTypeSpec( typeSpecs, values, location: string, - componentName, - element, - // It is only safe to pass fiber if it is the work-in-progress version, and - // only during reconciliation (begin and complete phase). - workInProgressOrDebugID + componentName ) { - function getStack() { - let stack = ''; - if (__DEV__) { - if (!ReactComponentTreeHook) { - ReactComponentTreeHook = require('ReactComponentTreeHook'); - } - if (workInProgressOrDebugID != null) { - if (typeof workInProgressOrDebugID === 'number') { - // DebugID from Stack. - const debugID = workInProgressOrDebugID; - stack = ReactComponentTreeHook.getStackAddendumByID(debugID); - } else if (typeof workInProgressOrDebugID.tag === 'number') { - // This is a Fiber. - // The stack will only be correct if this is a work in progress - // version and we're calling it during reconciliation. - const workInProgress = workInProgressOrDebugID; - stack = ReactComponentTreeHook.getStackAddendumByWorkInProgressFiber(workInProgress); - } - } else if (element !== null) { - stack = ReactComponentTreeHook.getCurrentStackAddendum(element); - } - } - return stack; - } - - checkPropTypes(typeSpecs, values, location, componentName, getStack); + checkPropTypes( + typeSpecs, + values, + location, + componentName, + getStackAddendum + ); } module.exports = checkReactTypeSpec; From 0320b8ecb44f4bfac44e27ae42ced1b805fc0e98 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 22 Feb 2017 14:51:27 -0800 Subject: [PATCH 8/9] Wrap checkPropTypes in DEV conditional so it's stripped out in prod --- .../classic/element/ReactDebugCurrentFrame.js | 46 +++++----- .../classic/types/checkPropTypes.js | 86 ++++++++++--------- 2 files changed, 66 insertions(+), 66 deletions(-) diff --git a/src/isomorphic/classic/element/ReactDebugCurrentFrame.js b/src/isomorphic/classic/element/ReactDebugCurrentFrame.js index f2e4676aeda..f4efdd672b6 100644 --- a/src/isomorphic/classic/element/ReactDebugCurrentFrame.js +++ b/src/isomorphic/classic/element/ReactDebugCurrentFrame.js @@ -15,44 +15,42 @@ import type { Fiber } from 'ReactFiber'; import type { DebugID } from 'ReactInstanceType'; +const ReactDebugCurrentFrame = {}; + if (__DEV__) { var { getStackAddendumByID, getStackAddendumByWorkInProgressFiber, getCurrentStackAddendum, } = require('ReactComponentTreeHook'); -} -const ReactDebugCurrentFrame = { // Component that is being worked on - current: (null : Fiber | DebugID | null), + ReactDebugCurrentFrame.current = (null : Fiber | DebugID | null); // Element that is being cloned or created - element: (null : *), + ReactDebugCurrentFrame.element = (null : *); - getStackAddendum() : string | null { + ReactDebugCurrentFrame.getStackAddendum = function() : string | null { let stack = null; - if (__DEV__) { - const current = ReactDebugCurrentFrame.current; - const element = ReactDebugCurrentFrame.element; - if (current !== null) { - if (typeof current === 'number') { - // DebugID from Stack. - const debugID = current; - stack = getStackAddendumByID(debugID); - } else if (typeof current.tag === 'number') { - // This is a Fiber. - // The stack will only be correct if this is a work in progress - // version and we're calling it during reconciliation. - const workInProgress = current; - stack = getStackAddendumByWorkInProgressFiber(workInProgress); - } - } else if (element !== null) { - stack = getCurrentStackAddendum(element); + const current = ReactDebugCurrentFrame.current; + const element = ReactDebugCurrentFrame.element; + if (current !== null) { + if (typeof current === 'number') { + // DebugID from Stack. + const debugID = current; + stack = getStackAddendumByID(debugID); + } else if (typeof current.tag === 'number') { + // This is a Fiber. + // The stack will only be correct if this is a work in progress + // version and we're calling it during reconciliation. + const workInProgress = current; + stack = getStackAddendumByWorkInProgressFiber(workInProgress); } + } else if (element !== null) { + stack = getCurrentStackAddendum(element); } return stack; - }, -}; + }; +} module.exports = ReactDebugCurrentFrame; diff --git a/src/isomorphic/classic/types/checkPropTypes.js b/src/isomorphic/classic/types/checkPropTypes.js index 172b96842e4..23ebacaf09a 100644 --- a/src/isomorphic/classic/types/checkPropTypes.js +++ b/src/isomorphic/classic/types/checkPropTypes.js @@ -30,53 +30,55 @@ var loggedTypeFailures = {}; * @private */ function checkPropTypes(typeSpecs, values, location, componentName, getStack) { - for (var typeSpecName in typeSpecs) { - if (typeSpecs.hasOwnProperty(typeSpecName)) { - var error; - // Prop type validation may throw. In case they do, we don't want to - // fail the render phase where it didn't fail before. So we log it. - // After these have been cleaned up, we'll let them throw. - try { - // This is intentionally an invariant that gets caught. It's the same - // behavior as without this statement except with a better message. - invariant( - typeof typeSpecs[typeSpecName] === 'function', - '%s: %s type `%s` is invalid; it must be a function, usually from ' + - 'React.PropTypes.', + if (__DEV__) { + for (var typeSpecName in typeSpecs) { + if (typeSpecs.hasOwnProperty(typeSpecName)) { + var error; + // Prop type validation may throw. In case they do, we don't want to + // fail the render phase where it didn't fail before. So we log it. + // After these have been cleaned up, we'll let them throw. + try { + // This is intentionally an invariant that gets caught. It's the same + // behavior as without this statement except with a better message. + invariant( + typeof typeSpecs[typeSpecName] === 'function', + '%s: %s type `%s` is invalid; it must be a function, usually from ' + + 'React.PropTypes.', + componentName || 'React class', + location, + typeSpecName + ); + error = typeSpecs[typeSpecName](values, typeSpecName, componentName, location, null, ReactPropTypesSecret); + } catch (ex) { + error = ex; + } + warning( + !error || error instanceof Error, + '%s: type specification of %s `%s` is invalid; the type checker ' + + 'function must return `null` or an `Error` but returned a %s. ' + + 'You may have forgotten to pass an argument to the type checker ' + + 'creator (arrayOf, instanceOf, objectOf, oneOf, oneOfType, and ' + + 'shape all require an argument).', componentName || 'React class', location, - typeSpecName + typeSpecName, + typeof error ); - error = typeSpecs[typeSpecName](values, typeSpecName, componentName, location, null, ReactPropTypesSecret); - } catch (ex) { - error = ex; - } - warning( - !error || error instanceof Error, - '%s: type specification of %s `%s` is invalid; the type checker ' + - 'function must return `null` or an `Error` but returned a %s. ' + - 'You may have forgotten to pass an argument to the type checker ' + - 'creator (arrayOf, instanceOf, objectOf, oneOf, oneOfType, and ' + - 'shape all require an argument).', - componentName || 'React class', - location, - typeSpecName, - typeof error - ); - if (error instanceof Error && !(error.message in loggedTypeFailures)) { - // Only monitor this failure once because there tends to be a lot of the - // same error. - loggedTypeFailures[error.message] = true; + if (error instanceof Error && !(error.message in loggedTypeFailures)) { + // Only monitor this failure once because there tends to be a lot of the + // same error. + loggedTypeFailures[error.message] = true; - var stack = getStack ? getStack() : ''; + var stack = getStack ? getStack() : ''; - warning( - false, - 'Failed %s type: %s%s', - location, - error.message, - stack != null ? stack : '', - ); + warning( + false, + 'Failed %s type: %s%s', + location, + error.message, + stack != null ? stack : '', + ); + } } } } From 03ed3437c988f294045770bf403839603ce00ae9 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 22 Feb 2017 15:01:40 -0800 Subject: [PATCH 9/9] Fix build config ReactDebugCurrentFrame is shared state. checkPropTypes should be imported via the main React export, not imported directly. --- gulpfile.js | 2 ++ src/isomorphic/React.js | 3 ++ .../classic/types/ReactPropTypes.js | 3 -- .../types/__tests__/ReactPropTypes-test.js | 34 ++++++++++--------- src/shared/types/checkReactTypeSpec.js | 1 - 5 files changed, 23 insertions(+), 20 deletions(-) diff --git a/gulpfile.js b/gulpfile.js index 71fd70c9ca1..731c459a951 100644 --- a/gulpfile.js +++ b/gulpfile.js @@ -130,7 +130,9 @@ var rendererSharedState = { React: 'react/lib/React', // Shared state ReactCurrentOwner: 'react/lib/ReactCurrentOwner', + checkPropTypes: 'react/lib/checkPropTypes', ReactComponentTreeHook: 'react/lib/ReactComponentTreeHook', + ReactDebugCurrentFrame: 'react/lib/ReactDebugCurrentFrame', }; var moduleMapReactDOM = Object.assign( diff --git a/src/isomorphic/React.js b/src/isomorphic/React.js index cb2c5a8d9cb..1d2f1b6e8e2 100644 --- a/src/isomorphic/React.js +++ b/src/isomorphic/React.js @@ -21,6 +21,7 @@ var ReactVersion = require('ReactVersion'); var onlyChild = require('onlyChild'); var warning = require('warning'); +var checkPropTypes = require('checkPropTypes'); var createElement = ReactElement.createElement; var createFactory = ReactElement.createFactory; @@ -71,6 +72,8 @@ var React = { cloneElement: cloneElement, isValidElement: ReactElement.isValidElement, + checkPropTypes: checkPropTypes, + // Classic PropTypes: ReactPropTypes, diff --git a/src/isomorphic/classic/types/ReactPropTypes.js b/src/isomorphic/classic/types/ReactPropTypes.js index 0225942cdd2..6f749170bc0 100644 --- a/src/isomorphic/classic/types/ReactPropTypes.js +++ b/src/isomorphic/classic/types/ReactPropTypes.js @@ -13,7 +13,6 @@ var ReactElement = require('ReactElement'); var ReactPropTypesSecret = require('ReactPropTypesSecret'); -var checkPropTypes = require('checkPropTypes'); var emptyFunction = require('emptyFunction'); var getIteratorFn = require('getIteratorFn'); @@ -123,8 +122,6 @@ if (__DEV__) { }; } -ReactPropTypes.checkPropTypes = checkPropTypes; - /** * inlined Object.is polyfill to avoid requiring consumers ship their own diff --git a/src/isomorphic/classic/types/__tests__/ReactPropTypes-test.js b/src/isomorphic/classic/types/__tests__/ReactPropTypes-test.js index b8f6aefb960..8ed0aa07ec4 100644 --- a/src/isomorphic/classic/types/__tests__/ReactPropTypes-test.js +++ b/src/isomorphic/classic/types/__tests__/ReactPropTypes-test.js @@ -12,10 +12,11 @@ 'use strict'; var PropTypes; +var checkPropTypes; var checkReactTypeSpec; var React; +var ReactDOM; var ReactFragment; -var ReactTestUtils; var Component; var MyComponent; @@ -23,6 +24,7 @@ var MyComponent; function resetWarningCache() { jest.resetModules(); checkReactTypeSpec = require('checkReactTypeSpec'); + checkPropTypes = require('checkPropTypes'); } function getPropTypeWarningMessage(propTypes, object, componentName) { @@ -109,8 +111,8 @@ describe('ReactPropTypes', () => { beforeEach(() => { PropTypes = require('ReactPropTypes'); React = require('React'); + ReactDOM = require('ReactDOM'); ReactFragment = require('ReactFragment'); - ReactTestUtils = require('ReactTestUtils'); resetWarningCache(); }); @@ -123,7 +125,7 @@ describe('ReactPropTypes', () => { }, }; const props = { foo: 'foo' }; - const returnValue = PropTypes.checkPropTypes(propTypes, props, 'prop', 'testComponent', null); + const returnValue = checkPropTypes(propTypes, props, 'prop', 'testComponent', null); expect(console.error.calls.argsFor(0)[0]).toContain('some error'); expect(returnValue).toBe(undefined); }); @@ -136,7 +138,7 @@ describe('ReactPropTypes', () => { }, }; const props = { foo: 'foo' }; - const returnValue = PropTypes.checkPropTypes(propTypes, props, 'prop', 'testComponent', null); + const returnValue = checkPropTypes(propTypes, props, 'prop', 'testComponent', null); expect(console.error.calls.argsFor(0)[0]).toContain('some error'); expect(returnValue).toBe(undefined); }); @@ -436,8 +438,8 @@ describe('ReactPropTypes', () => { it('should be able to define a single child as label', () => { spyOn(console, 'error'); - var instance = } />; - ReactTestUtils.renderIntoDocument(instance); + var container = document.createElement('div'); + ReactDOM.render(} />, container); expectDev(console.error.calls.count()).toBe(0); }); @@ -445,8 +447,8 @@ describe('ReactPropTypes', () => { it('should warn when passing no label and isRequired is set', () => { spyOn(console, 'error'); - var instance = ; - ReactTestUtils.renderIntoDocument(instance); + var container = document.createElement('div'); + ReactDOM.render(, container); expectDev(console.error.calls.count()).toBe(1); }); @@ -1064,8 +1066,8 @@ describe('ReactPropTypes', () => { } }; - var instance = ; - ReactTestUtils.renderIntoDocument(instance); + var container = document.createElement('div'); + ReactDOM.render(, container); expect(spy.calls.count()).toBe(1); expect(spy.calls.argsFor(0)[1]).toBe('num'); @@ -1081,8 +1083,8 @@ describe('ReactPropTypes', () => { } }; - var instance = ; - ReactTestUtils.renderIntoDocument(instance); + var container = document.createElement('div'); + ReactDOM.render(, container); expect(spy.calls.count()).toBe(1); expect(spy.calls.argsFor(0)[1]).toBe('num'); @@ -1105,8 +1107,8 @@ describe('ReactPropTypes', () => { } }; - var instance = ; - ReactTestUtils.renderIntoDocument(instance); + var container = document.createElement('div'); + ReactDOM.render(, container); expectDev(console.error.calls.count()).toBe(1); expect( console.error.calls.argsFor(0)[0].replace(/\(at .+?:\d+\)/g, '(at **)') @@ -1132,8 +1134,8 @@ describe('ReactPropTypes', () => { } }; - var instance = ; - ReactTestUtils.renderIntoDocument(instance); + var container = document.createElement('div'); + ReactDOM.render(, container); expectDev(console.error.calls.count()).toBe(0); } ); diff --git a/src/shared/types/checkReactTypeSpec.js b/src/shared/types/checkReactTypeSpec.js index f52fe68e477..c7ed4d8167e 100644 --- a/src/shared/types/checkReactTypeSpec.js +++ b/src/shared/types/checkReactTypeSpec.js @@ -12,7 +12,6 @@ 'use strict'; var checkPropTypes = require('checkPropTypes'); - var { getStackAddendum } = require('ReactDebugCurrentFrame'); function checkReactTypeSpec(