From d17bd176a2dd69b7f94b3cd0aa5b552a7fd239e8 Mon Sep 17 00:00:00 2001 From: Marshall Roch Date: Wed, 4 Dec 2013 17:47:58 -0800 Subject: [PATCH] Remove support for putting mixins, propTypes, etc. in 'statics' This leaves statics as a way to define static methods and properties that will be accessible on the return value of `React.createClass` without changing the current spec. --- src/core/ReactCompositeComponent.js | 153 ++++++++---------- .../__tests__/ReactCompositeComponent-test.js | 118 +++++--------- 2 files changed, 111 insertions(+), 160 deletions(-) diff --git a/src/core/ReactCompositeComponent.js b/src/core/ReactCompositeComponent.js index 6376cb8af2..0e3ef6fb49 100644 --- a/src/core/ReactCompositeComponent.js +++ b/src/core/ReactCompositeComponent.js @@ -85,39 +85,46 @@ var SpecPolicy = keyMirror({ */ var ReactCompositeComponentInterface = { - statics: { - /** - * An array of Mixin objects to include when defining your component. - * - * @type {array} - * @optional - */ - mixins: SpecPolicy.DEFINE_MANY, + /** + * An array of Mixin objects to include when defining your component. + * + * @type {array} + * @optional + */ + mixins: SpecPolicy.DEFINE_MANY, - /** - * Definition of prop types for this component. - * - * @type {object} - * @optional - */ - propTypes: SpecPolicy.DEFINE_MANY_MERGED, + /** + * An object containing properties and methods that should be defined on + * the component's constructor instead of its prototype (static methods). + * + * @type {object} + * @optional + */ + statics: SpecPolicy.DEFINE_MANY_MERGED, - /** - * Definition of context types for this component. - * - * @type {object} - * @optional - */ - contextTypes: SpecPolicy.DEFINE_MANY_MERGED, + /** + * Definition of prop types for this component. + * + * @type {object} + * @optional + */ + propTypes: SpecPolicy.DEFINE_MANY_MERGED, - /** - * Definition of context types this component sets for its children. - * - * @type {object} - * @optional - */ - childContextTypes: SpecPolicy.DEFINE_MANY_MERGED - }, + /** + * Definition of context types for this component. + * + * @type {object} + * @optional + */ + contextTypes: SpecPolicy.DEFINE_MANY_MERGED, + + /** + * Definition of context types this component sets for its children. + * + * @type {object} + * @optional + */ + childContextTypes: SpecPolicy.DEFINE_MANY_MERGED, // ==== Definition methods ==== @@ -304,21 +311,18 @@ var ReactCompositeComponentInterface = { }; /** - * Mapping from class specification static keys to special processing functions. + * Mapping from class specification keys to special processing functions. * - * These are all defined inside the "statics" key in the class spec: - * - * React.createClass({ - * statics: { - * mixins: [FooMixin] - * }, - * render: function() {...} - * }) - * - * Although these are declared in the specification when defining classes - * using `React.createClass`, they will not be on the component's prototype. + * Although these are declared like instance properties in the specification + * when defining classes using `React.createClass`, they are actually static + * and are accessible on the constructor instead of the prototype. Despite + * being static, they must be defined outside of the "statics" key under + * which all other static methods are defined. */ -var RESERVED_STATIC_SPEC_KEYS = { +var RESERVED_SPEC_KEYS = { + displayName: function(ConvenienceConstructor, displayName) { + ConvenienceConstructor.componentConstructor.displayName = displayName; + }, mixins: function(ConvenienceConstructor, mixins) { if (mixins) { for (var i = 0; i < mixins.length; i++) { @@ -352,22 +356,7 @@ var RESERVED_STATIC_SPEC_KEYS = { ReactPropTypeLocations.prop ); Constructor.propTypes = propTypes; - } -}; - -/** - * Mapping from class specification keys to special processing functions. - * - * Although these are declared in the specification when defining classes - * using `React.createClass`, they will not be on the component's prototype. - */ -var RESERVED_SPEC_KEYS = { - // TODO: move this to RESERVED_STATIC_SPEC_KEYS, but this requires also - // changing the JSX transform. - displayName: function(ConvenienceConstructor, displayName) { - ConvenienceConstructor.componentConstructor.displayName = displayName; }, - statics: function(ConvenienceConstructor, statics) { mixStaticSpecIntoComponent(ConvenienceConstructor, statics); } @@ -388,13 +377,11 @@ function validateTypeDef(Constructor, typeDef, location) { } } -function validateMethodOverride(proto, name, isStatic) { - var specPolicy = isStatic ? - ReactCompositeComponentInterface.statics[name] : - ReactCompositeComponentInterface[name]; +function validateMethodOverride(proto, name) { + var specPolicy = ReactCompositeComponentInterface[name]; // Disallow overriding of base class methods unless explicitly allowed. - if (!isStatic && ReactCompositeComponentMixin.hasOwnProperty(name)) { + if (ReactCompositeComponentMixin.hasOwnProperty(name)) { invariant( specPolicy === SpecPolicy.OVERRIDE_BASE, 'ReactCompositeComponentInterface: You are attempting to override ' + @@ -448,13 +435,10 @@ function mixSpecIntoComponent(ConvenienceConstructor, spec) { continue; } - validateMethodOverride(proto, name, false); + validateMethodOverride(proto, name); if (RESERVED_SPEC_KEYS.hasOwnProperty(name)) { RESERVED_SPEC_KEYS[name](ConvenienceConstructor, property); - } else if (RESERVED_STATIC_SPEC_KEYS.hasOwnProperty(name)) { - // TODO: deprecate this with a warning - RESERVED_STATIC_SPEC_KEYS[name](ConvenienceConstructor, property); } else { // Setup methods on prototype: // The following member methods should not be automatically bound: @@ -504,25 +488,24 @@ function mixStaticSpecIntoComponent(ConvenienceConstructor, statics) { return; } - validateMethodOverride(ConvenienceConstructor, name, true); - - if (RESERVED_STATIC_SPEC_KEYS.hasOwnProperty(name)) { - RESERVED_STATIC_SPEC_KEYS[name](ConvenienceConstructor, property); - } else { - var isInherited = name in ConvenienceConstructor; - var result = property; - if (isInherited) { - var existingProperty = ConvenienceConstructor[name]; - if (ReactCompositeComponentInterface.statics[name] === - SpecPolicy.DEFINE_MANY_MERGED) { - result = createMergedResultFunction(existingProperty, property); - } else { - result = createChainedFunction(existingProperty, property); - } - } - ConvenienceConstructor[name] = result; - ConvenienceConstructor.componentConstructor[name] = result; + var isInherited = name in ConvenienceConstructor; + var result = property; + if (isInherited) { + var existingProperty = ConvenienceConstructor[name]; + var existingType = typeof existingProperty; + var propertyType = typeof property; + invariant( + existingType === 'function' && propertyType === 'function', + 'ReactCompositeComponent: You are attempting to define ' + + '`%s` on your component more than once, but that is only supported ' + + 'for functions, which are chained together. This conflict may be ' + + 'due to a mixin.', + name + ); + result = createChainedFunction(existingProperty, property); } + ConvenienceConstructor[name] = result; + ConvenienceConstructor.componentConstructor[name] = result; } } diff --git a/src/core/__tests__/ReactCompositeComponent-test.js b/src/core/__tests__/ReactCompositeComponent-test.js index ed5a26118b..8869c2d252 100644 --- a/src/core/__tests__/ReactCompositeComponent-test.js +++ b/src/core/__tests__/ReactCompositeComponent-test.js @@ -235,9 +235,7 @@ describe('ReactCompositeComponent', function() { it('should normalize props with default values', function() { var Component = React.createClass({ - statics: { - propTypes: {key: ReactPropTypes.string.isRequired} - }, + propTypes: {key: ReactPropTypes.string.isRequired}, getDefaultProps: function() { return {key: 'testKey'}; }, @@ -264,9 +262,7 @@ describe('ReactCompositeComponent', function() { it('should check default prop values', function() { var Component = React.createClass({ - statics: { - propTypes: {key: ReactPropTypes.string.isRequired} - }, + propTypes: {key: ReactPropTypes.string.isRequired}, getDefaultProps: function() { return {key: null}; }, @@ -286,10 +282,8 @@ describe('ReactCompositeComponent', function() { it('should check declared prop types', function() { var Component = React.createClass({ - statics: { - propTypes: { - key: ReactPropTypes.string.isRequired - } + propTypes: { + key: ReactPropTypes.string.isRequired }, render: function() { return {this.props.key}; @@ -319,10 +313,8 @@ describe('ReactCompositeComponent', function() { expect(function() { React.createClass({ displayName: 'Component', - statics: { - propTypes: { - key: null - } + propTypes: { + key: null }, render: function() { return {this.props.key}; @@ -338,10 +330,8 @@ describe('ReactCompositeComponent', function() { expect(function() { React.createClass({ displayName: 'Component', - statics: { - contextTypes: { - key: null - } + contextTypes: { + key: null }, render: function() { return {this.props.key}; @@ -357,10 +347,8 @@ describe('ReactCompositeComponent', function() { expect(function() { React.createClass({ displayName: 'Component', - statics: { - childContextTypes: { - key: null - } + childContextTypes: { + key: null }, render: function() { return {this.props.key}; @@ -428,9 +416,7 @@ describe('ReactCompositeComponent', function() { } }; var Component = React.createClass({ - statics: { - mixins: [Mixin] - }, + mixins: [Mixin], getInitialState: function() { return {component: true}; }, @@ -451,9 +437,7 @@ describe('ReactCompositeComponent', function() { } }; var Component = React.createClass({ - statics: { - mixins: [Mixin] - }, + mixins: [Mixin], getInitialState: function() { return {x: true}; }, @@ -477,9 +461,7 @@ describe('ReactCompositeComponent', function() { } }; var Component = React.createClass({ - statics: { - mixins: [Mixin] - }, + mixins: [Mixin], getInitialState: function() { return {x: true}; }, @@ -614,11 +596,9 @@ describe('ReactCompositeComponent', function() { var grandchildInstance = null; var Parent = React.createClass({ - statics: { - childContextTypes: { - foo: ReactPropTypes.string, - depth: ReactPropTypes.number - } + childContextTypes: { + foo: ReactPropTypes.string, + depth: ReactPropTypes.number }, getChildContext: function() { @@ -635,15 +615,13 @@ describe('ReactCompositeComponent', function() { }); var Child = React.createClass({ - statics: { - contextTypes: { - foo: ReactPropTypes.string, - depth: ReactPropTypes.number - }, + contextTypes: { + foo: ReactPropTypes.string, + depth: ReactPropTypes.number + }, - childContextTypes: { - depth: ReactPropTypes.number - } + childContextTypes: { + depth: ReactPropTypes.number }, getChildContext: function() { @@ -659,11 +637,9 @@ describe('ReactCompositeComponent', function() { }); var Grandchild = React.createClass({ - statics: { - contextTypes: { - foo: ReactPropTypes.string, - depth: ReactPropTypes.number - } + contextTypes: { + foo: ReactPropTypes.string, + depth: ReactPropTypes.number }, render: function() { @@ -679,10 +655,8 @@ describe('ReactCompositeComponent', function() { it('should check context types', function() { var Component = React.createClass({ - statics: { - contextTypes: { - foo: ReactPropTypes.string.isRequired - } + contextTypes: { + foo: ReactPropTypes.string.isRequired }, render: function() { @@ -715,11 +689,9 @@ describe('ReactCompositeComponent', function() { it('should check child context types', function() { var Component = React.createClass({ - statics: { - childContextTypes: { - foo: ReactPropTypes.string.isRequired, - bar: ReactPropTypes.number - } + childContextTypes: { + foo: ReactPropTypes.string.isRequired, + bar: ReactPropTypes.number }, getChildContext: function() { @@ -764,10 +736,8 @@ describe('ReactCompositeComponent', function() { it('should filter out context not in contextTypes', function() { var Component = React.createClass({ - statics: { - contextTypes: { - foo: ReactPropTypes.string - } + contextTypes: { + foo: ReactPropTypes.string }, render: function() { @@ -789,11 +759,9 @@ describe('ReactCompositeComponent', function() { var actualComponentDidUpdate; var Parent = React.createClass({ - statics: { - childContextTypes: { - foo: ReactPropTypes.string.isRequired, - bar: ReactPropTypes.string.isRequired - } + childContextTypes: { + foo: ReactPropTypes.string.isRequired, + bar: ReactPropTypes.string.isRequired }, getChildContext: function() { @@ -809,10 +777,8 @@ describe('ReactCompositeComponent', function() { }); var Component = React.createClass({ - statics: { - contextTypes: { - foo: ReactPropTypes.string - } + contextTypes: { + foo: ReactPropTypes.string }, componentWillReceiveProps: function(nextProps, nextContext) { @@ -896,8 +862,9 @@ describe('ReactCompositeComponent', function() { } }; React.createClass({ + mixins: [Mixin], + statics: { - mixins: [Mixin], abc: 'bar' }, @@ -906,9 +873,10 @@ describe('ReactCompositeComponent', function() { } }); }).toThrow( - 'Invariant Violation: ReactCompositeComponentInterface: You are ' + - 'attempting to define `abc` on your component more than once. This ' + - 'conflict may be due to a mixin.' + 'Invariant Violation: ReactCompositeComponent: You are attempting to ' + + 'define `abc` on your component more than once, but that is only ' + + 'supported for functions, which are chained together. This conflict ' + + 'may be due to a mixin.' ); }); });