Skip to content

Warn when defined methods are used in plain JS classes #2806

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
sebmarkbage merged 1 commit into from
Jan 13, 2015
Merged

Warn when defined methods are used in plain JS classes #2806

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 14 additions & 4 deletions src/classic/class/ReactClass.js
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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