Skip to content

Commit

Permalink
Merge pull request #2806 from sebmarkbage/baseclass
Browse files Browse the repository at this point in the history 
Warn when defined methods are used in plain JS classes
  • Loading branch information
@sebmarkbage
sebmarkbage committed Jan 13, 2015
2 parents d138f9a + 2330962 commit 7a3083a
Show file tree
Hide file tree
Showing 3 changed files with 139 additions and 16 deletions.
18 changes: 14 additions & 4 deletions src/classic/class/ReactClass.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -808,12 +808,22 @@ var ReactClass = {
// Initialize the defaultProps property after all mixins have been merged
if (Constructor.getDefaultProps) {
Constructor.defaultProps = Constructor.getDefaultProps();
if (__DEV__) {
// This is a tag to indicate that this use of getDefaultProps is ok,
// since it's used with createClass. If it's not, then it's likely a
// mistake so we'll warn you to use the static property instead.
}

if (__DEV__) {
// This is a tag to indicate that the use of these method names is ok,
// since it's used with createClass. If it's not, then it's likely a
// mistake so we'll warn you to use the static property, property
// initializer or constructor respectively.
if (Constructor.getDefaultProps) {
Constructor.getDefaultProps._isReactClassApproved = true;
}
if (Constructor.prototype.getInitialState) {
Constructor.prototype.getInitialState._isReactClassApproved = true;
}
if (Constructor.prototype.componentWillMount) {
Constructor.prototype.componentWillMount._isReactClassApproved = true;
}
}

invariant(
Expand Down
76 changes: 64 additions & 12 deletions src/core/ReactCompositeComponent.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,7 @@ var warning = require('warning');
function getDeclarationErrorAddendum(component) {
var owner = component._currentElement._owner || null;
if (owner) {
var constructor = owner._instance.constructor;
var name = constructor && (constructor.displayName || constructor.name);
var name = owner.getName();
if (name) {
return ' Check the render method of `' + name + '`.';
}
Expand Down Expand Up @@ -200,11 +199,50 @@ var ReactCompositeComponentMixin = assign({},
// deprecating this convenience.
initialState = null;
}
// Since plain JS classes are defined without any special initialization
// logic, we can not catch common errors early. Therefore, we have to
// catch them here, at initialization time, instead.
warning(
!inst.getInitialState ||
inst.getInitialState._isReactClassApproved,
'getInitialState was defined on %s, a plain JavaScript class. ' +
'This is only supported for classes created using React.createClass. ' +
'Did you mean to define a state property instead?',
this.getName() || 'a component'
);
warning(
!inst.componentWillMount ||
inst.componentWillMount._isReactClassApproved,
'componentWillMount was defined on %s, a plain JavaScript class. ' +
'This is only supported for classes created using React.createClass. ' +
'Did you mean to define a constructor instead?',
this.getName() || 'a component'
);
warning(
!inst.propTypes,
'propTypes was defined as an instance property on %s. Use a static ' +
'property to define propTypes instead.',
this.getName() || 'a component'
);
warning(
!inst.contextTypes,
'contextTypes was defined as an instance property on %s. Use a ' +
'static property to define contextTypes instead.',
this.getName() || 'a component'
);
warning(
typeof inst.componentShouldUpdate !== 'function',
'%s has a method called ' +
'componentShouldUpdate(). Did you mean shouldComponentUpdate()? ' +
'The name is phrased as a question because the function is ' +
'expected to return a value.',
(this.getName() || 'A component')
);
}
invariant(
typeof initialState === 'object' && !Array.isArray(initialState),
'%s.getInitialState(): must return an object or null',
inst.constructor.displayName || 'ReactCompositeComponent'
this.getName() || 'ReactCompositeComponent'
);
inst.state = initialState;

Expand Down Expand Up @@ -453,13 +491,12 @@ var ReactCompositeComponentMixin = assign({},
_processChildContext: function(currentContext) {
var inst = this._instance;
var childContext = inst.getChildContext && inst.getChildContext();
var displayName = inst.constructor.displayName || 'ReactCompositeComponent';
if (childContext) {
invariant(
typeof inst.constructor.childContextTypes === 'object',
'%s.getChildContext(): childContextTypes must be defined in order to ' +
'use getChildContext().',
displayName
this.getName() || 'ReactCompositeComponent'
);
if (__DEV__) {
this._checkPropTypes(
Expand All @@ -472,7 +509,7 @@ var ReactCompositeComponentMixin = assign({},
invariant(
name in inst.constructor.childContextTypes,
'%s.getChildContext(): key "%s" is not defined in childContextTypes.',
displayName,
this.getName() || 'ReactCompositeComponent',
name
);
}
Expand Down Expand Up @@ -512,8 +549,7 @@ var ReactCompositeComponentMixin = assign({},
_checkPropTypes: function(propTypes, props, location) {
// TODO: Stop validating prop types here and only use the element
// validation.
var componentName = this._instance.constructor.displayName ||
this._instance.constructor.name;
var componentName = this.getName();
for (var propName in propTypes) {
if (propTypes.hasOwnProperty(propName)) {
var error;
Expand Down Expand Up @@ -614,7 +650,7 @@ var ReactCompositeComponentMixin = assign({},
_warnIfContextsDiffer: function(ownerBasedContext, parentBasedContext) {
var ownerKeys = Object.keys(ownerBasedContext).sort();
var parentKeys = Object.keys(parentBasedContext).sort();
var displayName = this._instance.constructor.displayName || 'ReactCompositeComponent';
var displayName = this.getName() || 'ReactCompositeComponent';
if (ownerKeys.length !== parentKeys.length ||
ownerKeys.toString() !== parentKeys.toString()) {
warning(
Expand Down Expand Up @@ -706,7 +742,7 @@ var ReactCompositeComponentMixin = assign({},
if (__DEV__) {
if (typeof shouldUpdate === "undefined") {
console.warn(
(inst.constructor.displayName || 'ReactCompositeComponent') +
(this.getName() || 'ReactCompositeComponent') +
'.shouldComponentUpdate(): Returned undefined instead of a ' +
'boolean value. Make sure to return true or false.'
);
Expand Down Expand Up @@ -863,7 +899,7 @@ var ReactCompositeComponentMixin = assign({},
ReactElement.isValidElement(renderedComponent),
'%s.render(): A valid ReactComponent must be returned. You may have ' +
'returned undefined, an array or some other invalid object.',
inst.constructor.displayName || 'ReactCompositeComponent'
this.getName() || 'ReactCompositeComponent'
);
return renderedComponent;
},
Expand Down Expand Up @@ -893,6 +929,22 @@ var ReactCompositeComponentMixin = assign({},
var refs = this.getPublicInstance().refs;
delete refs[ref];
},

/**
* Get a text description of the component that can be used to identify it
* in error messages.
* @return {string} The name or null.
* @internal
*/
getName: function() {
var type = this._currentElement.type;
var constructor = this._instance.constructor;
return (
type.displayName || (constructor && constructor.displayName) ||
type.name || (constructor && constructor.name) ||
null
);
},

/**
* Get the publicly accessible representation of this component - i.e. what
Expand Down Expand Up @@ -954,7 +1006,7 @@ var ShallowMixin = assign({},
invariant(
typeof initialState === 'object' && !Array.isArray(initialState),
'%s.getInitialState(): must return an object or null',
inst.constructor.displayName || 'ReactCompositeComponent'
this.getName() || 'ReactCompositeComponent'
);
inst.state = initialState;

Expand Down
61 changes: 61 additions & 0 deletions src/modern/class/__tests__/ReactES6Class-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,67 @@ describe('ReactES6Class', function() {
expect(renderedName).toBe('bar');
});

it('warns when classic properties are defined on the instance, ' +
'but does not invoke them.', function() {
spyOn(console, 'warn');
var getInitialStateWasCalled = false;
var componentWillMountWasCalled = false;
class Foo extends React.Component {
constructor() {
this.contextTypes = {};
this.propTypes = {};
}
getInitialState() {
getInitialStateWasCalled = true;
return {};
}
componentWillMount() {
componentWillMountWasCalled = true;
}
render() {
return <span className="foo" />;
}
}
test(<Foo />, 'SPAN', 'foo');
// TODO: expect(getInitialStateWasCalled).toBe(false);
// TODO: expect(componentWillMountWasCalled).toBe(false);
expect(console.warn.calls.length).toBe(4);
expect(console.warn.calls[0].args[0]).toContain(
'getInitialState was defined on Foo, a plain JavaScript class.'
);
expect(console.warn.calls[1].args[0]).toContain(
'componentWillMount was defined on Foo, a plain JavaScript class.'
);
expect(console.warn.calls[2].args[0]).toContain(
'propTypes was defined as an instance property on Foo.'
);
expect(console.warn.calls[3].args[0]).toContain(
'contextTypes was defined as an instance property on Foo.'
);
});

it('should warn when mispelling shouldComponentUpdate', function() {
spyOn(console, 'warn');

class NamedComponent {
componentShouldUpdate() {
return false;
}
render() {
return <span className="foo" />;
}
}
test(<NamedComponent />, 'SPAN', 'foo');

expect(console.warn.calls.length).toBe(1);
expect(console.warn.calls[0].args[0]).toBe(
'Warning: ' +
'NamedComponent has a method called componentShouldUpdate(). Did you ' +
'mean shouldComponentUpdate()? The name is phrased as a question ' +
'because the function is expected to return a value.'
);
});

it('should throw AND warn when trying to access classic APIs', function() {
spyOn(console, 'warn');
var instance = test(<Inner name="foo" />, 'DIV', 'foo');
Expand Down

0 comments on commit 7a3083a

Please sign in to comment.