From 2330962d25fdde9a3e0275aa4e44f572f77be07e Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Tue, 13 Jan 2015 11:01:10 -0800 Subject: [PATCH] Warn when defined methods are used in plain JS classes In ReactClass we use early validation to warn you if a accidentally defined propTypes in the wrong place or if you mispelled componentShouldUpdate. For plain JS classes there is no early validation process. Therefore, we wait to do this validation until the component is mounted before we issue the warning. This should bring us to warning-parity with ReactClass. --- src/classic/class/ReactClass.js | 18 ++++- src/core/ReactCompositeComponent.js | 76 ++++++++++++++++--- .../class/__tests__/ReactES6Class-test.js | 61 +++++++++++++++ 3 files changed, 139 insertions(+), 16 deletions(-) diff --git a/src/classic/class/ReactClass.js b/src/classic/class/ReactClass.js index f5340e2d083..09dd3452e9c 100644 --- a/src/classic/class/ReactClass.js +++ b/src/classic/class/ReactClass.js @@ -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( diff --git a/src/core/ReactCompositeComponent.js b/src/core/ReactCompositeComponent.js index ab78a4d345d..f2f321426a9 100644 --- a/src/core/ReactCompositeComponent.js +++ b/src/core/ReactCompositeComponent.js @@ -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 + '`.'; } @@ -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; @@ -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( @@ -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 ); } @@ -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; @@ -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( @@ -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.' ); @@ -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; }, @@ -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 @@ -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; diff --git a/src/modern/class/__tests__/ReactES6Class-test.js b/src/modern/class/__tests__/ReactES6Class-test.js index 1a78f397d54..b64e198864d 100644 --- a/src/modern/class/__tests__/ReactES6Class-test.js +++ b/src/modern/class/__tests__/ReactES6Class-test.js @@ -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 ; + } + } + test(, '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 ; + } + } + test(, '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(, 'DIV', 'foo');