Skip to content

Commit

Permalink
Move defaultProps resolution and type validation to the descriptor
Browse files Browse the repository at this point in the history 
This copies the propType and contextType validation to a wrapper around the
descriptor factory. By doing the validation early, we make it easier to track
down bugs. It also prepares for static type checking which should be done at the
usage site.

This validation is not yet active and is just logged using monitorCodeUse. This
will allow us to clean up callsites which would fail this new type of
validation.

I chose to copy the validation instead of abstracting it out to a common abstraction. This is just an
intermediate step to avoid spamming consoles. The original validation in the instance will be deleted as soon as we can turn on the warnings at the callsite. Copy+Delete makes this a more a much cleaner diff review/history.

Additionally, getDefaultProps are moved to become a static function which is
only executed once. It should be moved to statics but we don't have a
convenient way to merge mixins in statics right now. Deferring to ES6 classes.

This is still a breaking change since you can return an object or array from
getDefaultProps, which later gets mutated and now the shared instance is
mutated. Mutating an object that is passed into you from props is highly
discouraged and likely to lead to subtle bugs anyway. So I'm not too worried.

The defaultProps are now resolved in the descriptor factory. This will enable
a perf optimizations where we don't create an unnecessary object allocation
when you use default props. It also means that ReactChildren.map has access to
resolved properties which gives them consistent behavior whether or not the
default prop is specified.

This is a breaking change since it can affect how mapping over children and
transferPropsTo works together with defaultProps.
  • Loading branch information
@sebmarkbage @zpao
sebmarkbage authored and zpao committed May 6, 2014
1 parent b48a534 commit ff52e3d
Show file tree
Hide file tree
Showing 5 changed files with 386 additions and 205 deletions.
7 changes: 7 additions & 0 deletions src/browser/ReactDOM.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
"use strict";

var ReactDescriptor = require('ReactDescriptor');
var ReactDescriptorValidator = require('ReactDescriptorValidator');
var ReactDOMComponent = require('ReactDOMComponent');

var mergeInto = require('mergeInto');
Expand Down Expand Up @@ -50,6 +51,12 @@ function createDOMComponentClass(omitClose, tag) {

var ConvenienceConstructor = ReactDescriptor.createFactory(Constructor);

if (__DEV__) {
return ReactDescriptorValidator.createFactory(
ConvenienceConstructor
);
}

return ConvenienceConstructor;
}

Expand Down
82 changes: 45 additions & 37 deletions src/core/ReactCompositeComponent.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ var ReactComponent = require('ReactComponent');
var ReactContext = require('ReactContext');
var ReactCurrentOwner = require('ReactCurrentOwner');
var ReactDescriptor = require('ReactDescriptor');
var ReactDescriptorValidator = require('ReactDescriptorValidator');
var ReactEmptyComponent = require('ReactEmptyComponent');
var ReactErrorUtils = require('ReactErrorUtils');
var ReactOwner = require('ReactOwner');
Expand Down Expand Up @@ -328,18 +329,17 @@ var ReactCompositeComponentInterface = {
* which all other static methods are defined.
*/
var RESERVED_SPEC_KEYS = {
displayName: function(ConvenienceConstructor, displayName) {
ConvenienceConstructor.type.displayName = displayName;
displayName: function(Constructor, displayName) {
Constructor.displayName = displayName;
},
mixins: function(ConvenienceConstructor, mixins) {
mixins: function(Constructor, mixins) {
if (mixins) {
for (var i = 0; i < mixins.length; i++) {
mixSpecIntoComponent(ConvenienceConstructor, mixins[i]);
mixSpecIntoComponent(Constructor, mixins[i]);
}
}
},
childContextTypes: function(ConvenienceConstructor, childContextTypes) {
var Constructor = ConvenienceConstructor.type;
childContextTypes: function(Constructor, childContextTypes) {
validateTypeDef(
Constructor,
childContextTypes,
Expand All @@ -350,26 +350,38 @@ var RESERVED_SPEC_KEYS = {
childContextTypes
);
},
contextTypes: function(ConvenienceConstructor, contextTypes) {
var Constructor = ConvenienceConstructor.type;
contextTypes: function(Constructor, contextTypes) {
validateTypeDef(
Constructor,
contextTypes,
ReactPropTypeLocations.context
);
Constructor.contextTypes = merge(Constructor.contextTypes, contextTypes);
},
propTypes: function(ConvenienceConstructor, propTypes) {
var Constructor = ConvenienceConstructor.type;
/**
* Special case getDefaultProps which should move into statics but requires
* automatic merging.
*/
getDefaultProps: function(Constructor, getDefaultProps) {
if (Constructor.getDefaultProps) {
Constructor.getDefaultProps = createMergedResultFunction(
Constructor.getDefaultProps,
getDefaultProps
);
} else {
Constructor.getDefaultProps = getDefaultProps;
}
},
propTypes: function(Constructor, propTypes) {
validateTypeDef(
Constructor,
propTypes,
ReactPropTypeLocations.prop
);
Constructor.propTypes = merge(Constructor.propTypes, propTypes);
},
statics: function(ConvenienceConstructor, statics) {
mixStaticSpecIntoComponent(ConvenienceConstructor, statics);
statics: function(Constructor, statics) {
mixStaticSpecIntoComponent(Constructor, statics);
}
};

Expand Down Expand Up @@ -437,7 +449,7 @@ function validateLifeCycleOnReplaceState(instance) {
* Custom version of `mixInto` which handles policy validation and reserved
* specification keys when building `ReactCompositeComponent` classses.
*/
function mixSpecIntoComponent(ConvenienceConstructor, spec) {
function mixSpecIntoComponent(Constructor, spec) {
invariant(
!ReactDescriptor.isValidFactory(spec),
'ReactCompositeComponent: You\'re attempting to ' +
Expand All @@ -449,7 +461,6 @@ function mixSpecIntoComponent(ConvenienceConstructor, spec) {
'use a component as a mixin. Instead, just use a regular object.'
);

var Constructor = ConvenienceConstructor.type;
var proto = Constructor.prototype;
for (var name in spec) {
var property = spec[name];
Expand All @@ -460,7 +471,7 @@ function mixSpecIntoComponent(ConvenienceConstructor, spec) {
validateMethodOverride(proto, name);

if (RESERVED_SPEC_KEYS.hasOwnProperty(name)) {
RESERVED_SPEC_KEYS[name](ConvenienceConstructor, property);
RESERVED_SPEC_KEYS[name](Constructor, property);
} else {
// Setup methods on prototype:
// The following member methods should not be automatically bound:
Expand Down Expand Up @@ -521,7 +532,7 @@ function mixSpecIntoComponent(ConvenienceConstructor, spec) {
}
}

function mixStaticSpecIntoComponent(ConvenienceConstructor, statics) {
function mixStaticSpecIntoComponent(Constructor, statics) {
if (!statics) {
return;
}
Expand All @@ -531,10 +542,10 @@ function mixStaticSpecIntoComponent(ConvenienceConstructor, statics) {
continue;
}

var isInherited = name in ConvenienceConstructor;
var isInherited = name in Constructor;
var result = property;
if (isInherited) {
var existingProperty = ConvenienceConstructor[name];
var existingProperty = Constructor[name];
var existingType = typeof existingProperty;
var propertyType = typeof property;
invariant(
Expand All @@ -547,8 +558,7 @@ function mixStaticSpecIntoComponent(ConvenienceConstructor, statics) {
);
result = createChainedFunction(existingProperty, property);
}
ConvenienceConstructor[name] = result;
ConvenienceConstructor.type[name] = result;
Constructor[name] = result;
}
}

Expand Down Expand Up @@ -723,7 +733,6 @@ var ReactCompositeComponentMixin = {
this._compositeLifeCycleState = CompositeLifeCycle.MOUNTING;

this.context = this._processContext(this._descriptor._context);
this._defaultProps = this.getDefaultProps ? this.getDefaultProps() : null;
this.props = this._processProps(this.props);

if (this.__reactAutoBindMap) {
Expand Down Expand Up @@ -781,8 +790,6 @@ var ReactCompositeComponentMixin = {
}
this._compositeLifeCycleState = null;

this._defaultProps = null;

this._renderedComponent.unmountComponent();
this._renderedComponent = null;

Expand Down Expand Up @@ -921,12 +928,6 @@ var ReactCompositeComponentMixin = {
*/
_processProps: function(newProps) {
var props = merge(newProps);
var defaultProps = this._defaultProps;
for (var propName in defaultProps) {
if (typeof props[propName] === 'undefined') {
props[propName] = defaultProps[propName];
}
}
if (__DEV__) {
var propTypes = this.constructor.propTypes;
if (propTypes) {
Expand All @@ -945,6 +946,8 @@ var ReactCompositeComponentMixin = {
* @private
*/
_checkPropTypes: function(propTypes, props, location) {
// TODO: Stop validating prop types here and only use the descriptor
// validation.
var componentName = this.constructor.displayName;
for (var propName in propTypes) {
if (propTypes.hasOwnProperty(propName)) {
Expand Down Expand Up @@ -1310,16 +1313,11 @@ var ReactCompositeComponent = {
Constructor.prototype = new ReactCompositeComponentBase();
Constructor.prototype.constructor = Constructor;

var ConvenienceConstructor = ReactDescriptor.createFactory(Constructor);

// TODO: Move statics off of the convenience constructor. That way the
// factory can be created independently from the main class.

injectedMixins.forEach(
mixSpecIntoComponent.bind(null, ConvenienceConstructor)
mixSpecIntoComponent.bind(null, Constructor)
);

mixSpecIntoComponent(ConvenienceConstructor, spec);
mixSpecIntoComponent(Constructor, spec);

invariant(
Constructor.prototype.render,
Expand Down Expand Up @@ -1348,7 +1346,17 @@ var ReactCompositeComponent = {
}
}

return ConvenienceConstructor;
var descriptorFactory = ReactDescriptor.createFactory(Constructor);

if (__DEV__) {
return ReactDescriptorValidator.createFactory(
descriptorFactory,
Constructor.propTypes,
Constructor.contextTypes
);
}

return descriptorFactory;
},

injection: {
Expand Down
Loading

0 comments on commit ff52e3d

Please sign in to comment.