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.
Sebastian Markbage committed
Jan 13, 2015 at 11:01 UTC
2330962d25fdde9a3e0275aa4e44f572f77be07e
3 files changed
+139
-16
src/classic/class/ReactClass.js
+14
-4
@@ -808,12 +808,22 @@ var ReactClass = {
808
// Initialize the defaultProps property after all mixins have been merged
809
if (Constructor.getDefaultProps) {
810
Constructor.defaultProps = Constructor.getDefaultProps();
811
- if (__DEV__) {
812
- // This is a tag to indicate that this use of getDefaultProps is ok,
813
- // since it's used with createClass. If it's not, then it's likely a
814
- // mistake so we'll warn you to use the static property instead.
811
+ }
812
+
813
+ if (__DEV__) {
814
+ // This is a tag to indicate that the use of these method names is ok,
815
+ // since it's used with createClass. If it's not, then it's likely a
816
+ // mistake so we'll warn you to use the static property, property
817
+ // initializer or constructor respectively.
818
+ if (Constructor.getDefaultProps) {
819
Constructor.getDefaultProps._isReactClassApproved = true;
820
}
821
+ if (Constructor.prototype.getInitialState) {
822
+ Constructor.prototype.getInitialState._isReactClassApproved = true;
823
+ }
824
+ if (Constructor.prototype.componentWillMount) {
825
+ Constructor.prototype.componentWillMount._isReactClassApproved = true;
826
+ }
827
}
828
829
invariant(
src/core/ReactCompositeComponent.js
+64
-12
@@ -32,8 +32,7 @@ var warning = require('warning');
32
function getDeclarationErrorAddendum(component) {
33
var owner = component._currentElement._owner || null;
34
if (owner) {
35
- var constructor = owner._instance.constructor;
36
- var name = constructor && (constructor.displayName || constructor.name);
35
+ var name = owner.getName();
36
if (name) {
37
return ' Check the render method of `' + name + '`.';
38
}
@@ -200,11 +199,50 @@ var ReactCompositeComponentMixin = assign({},
199
// deprecating this convenience.
200
initialState = null;
201
}
202
+ // Since plain JS classes are defined without any special initialization
203
+ // logic, we can not catch common errors early. Therefore, we have to
204
+ // catch them here, at initialization time, instead.
205
+ warning(
206
+ !inst.getInitialState ||
207
+ inst.getInitialState._isReactClassApproved,
208
+ 'getInitialState was defined on %s, a plain JavaScript class. ' +
209
+ 'This is only supported for classes created using React.createClass. ' +
210
+ 'Did you mean to define a state property instead?',
211
+ this.getName() || 'a component'
212
+ );
213
+ warning(
214
+ !inst.componentWillMount ||
215
+ inst.componentWillMount._isReactClassApproved,
216
+ 'componentWillMount was defined on %s, a plain JavaScript class. ' +
217
+ 'This is only supported for classes created using React.createClass. ' +
218
+ 'Did you mean to define a constructor instead?',
219
+ this.getName() || 'a component'
220
+ );
221
+ warning(
222
+ !inst.propTypes,
223
+ 'propTypes was defined as an instance property on %s. Use a static ' +
224
+ 'property to define propTypes instead.',
225
+ this.getName() || 'a component'
226
+ );
227
+ warning(
228
+ !inst.contextTypes,
229
+ 'contextTypes was defined as an instance property on %s. Use a ' +
230
+ 'static property to define contextTypes instead.',
231
+ this.getName() || 'a component'
232
+ );
233
+ warning(
234
+ typeof inst.componentShouldUpdate !== 'function',
235
+ '%s has a method called ' +
236
+ 'componentShouldUpdate(). Did you mean shouldComponentUpdate()? ' +
237
+ 'The name is phrased as a question because the function is ' +
238
+ 'expected to return a value.',
239
+ (this.getName() || 'A component')
240
+ );
241
}
242
invariant(
243
typeof initialState === 'object' && !Array.isArray(initialState),
244
'%s.getInitialState(): must return an object or null',
207
- inst.constructor.displayName || 'ReactCompositeComponent'
245
+ this.getName() || 'ReactCompositeComponent'
246
);
247
inst.state = initialState;
248
@@ -453,13 +491,12 @@ var ReactCompositeComponentMixin = assign({},
491
_processChildContext: function(currentContext) {
492
var inst = this._instance;
493
var childContext = inst.getChildContext && inst.getChildContext();
456
- var displayName = inst.constructor.displayName || 'ReactCompositeComponent';
494
if (childContext) {
495
invariant(
496
typeof inst.constructor.childContextTypes === 'object',
497
'%s.getChildContext(): childContextTypes must be defined in order to ' +
498
'use getChildContext().',
462
- displayName
499
+ this.getName() || 'ReactCompositeComponent'
500
);
501
if (__DEV__) {
502
this._checkPropTypes(
@@ -472,7 +509,7 @@ var ReactCompositeComponentMixin = assign({},
509
invariant(
510
name in inst.constructor.childContextTypes,
511
'%s.getChildContext(): key "%s" is not defined in childContextTypes.',
475
- displayName,
512
+ this.getName() || 'ReactCompositeComponent',
513
name
514
);
515
}
@@ -512,8 +549,7 @@ var ReactCompositeComponentMixin = assign({},
549
_checkPropTypes: function(propTypes, props, location) {
550
// TODO: Stop validating prop types here and only use the element
551
// validation.
515
- var componentName = this._instance.constructor.displayName ||
516
- this._instance.constructor.name;
552
+ var componentName = this.getName();
553
for (var propName in propTypes) {
554
if (propTypes.hasOwnProperty(propName)) {
555
var error;
@@ -614,7 +650,7 @@ var ReactCompositeComponentMixin = assign({},
650
_warnIfContextsDiffer: function(ownerBasedContext, parentBasedContext) {
651
var ownerKeys = Object.keys(ownerBasedContext).sort();
652
var parentKeys = Object.keys(parentBasedContext).sort();
617
- var displayName = this._instance.constructor.displayName || 'ReactCompositeComponent';
653
+ var displayName = this.getName() || 'ReactCompositeComponent';
654
if (ownerKeys.length !== parentKeys.length ||
655
ownerKeys.toString() !== parentKeys.toString()) {
656
warning(
@@ -706,7 +742,7 @@ var ReactCompositeComponentMixin = assign({},
742
if (__DEV__) {
743
if (typeof shouldUpdate === "undefined") {
744
console.warn(
709
- (inst.constructor.displayName || 'ReactCompositeComponent') +
745
+ (this.getName() || 'ReactCompositeComponent') +
746
'.shouldComponentUpdate(): Returned undefined instead of a ' +
747
'boolean value. Make sure to return true or false.'
748
);
@@ -863,7 +899,7 @@ var ReactCompositeComponentMixin = assign({},
899
ReactElement.isValidElement(renderedComponent),
900
'%s.render(): A valid ReactComponent must be returned. You may have ' +
901
'returned undefined, an array or some other invalid object.',
866
- inst.constructor.displayName || 'ReactCompositeComponent'
902
+ this.getName() || 'ReactCompositeComponent'
903
);
904
return renderedComponent;
905
},
@@ -893,6 +929,22 @@ var ReactCompositeComponentMixin = assign({},
929
var refs = this.getPublicInstance().refs;
930
delete refs[ref];
931
},
932
+
933
+ /**
934
+ * Get a text description of the component that can be used to identify it
935
+ * in error messages.
936
+ * @return {string} The name or null.
937
+ * @internal
938
+ */
939
+ getName: function() {
940
+ var type = this._currentElement.type;
941
+ var constructor = this._instance.constructor;
942
+ return (
943
+ type.displayName || (constructor && constructor.displayName) ||
944
+ type.name || (constructor && constructor.name) ||
945
+ null
946
+ );
947
+ },
948
949
/**
950
* Get the publicly accessible representation of this component - i.e. what
@@ -954,7 +1006,7 @@ var ShallowMixin = assign({},
1006
invariant(
1007
typeof initialState === 'object' && !Array.isArray(initialState),
1008
'%s.getInitialState(): must return an object or null',
957
- inst.constructor.displayName || 'ReactCompositeComponent'
1009
+ this.getName() || 'ReactCompositeComponent'
1010
);
1011
inst.state = initialState;
1012
src/modern/class/__tests__/ReactES6Class-test.js
+61
@@ -90,6 +90,67 @@ describe('ReactES6Class', function() {
90
expect(renderedName).toBe('bar');
91
});
92
93
+ it('warns when classic properties are defined on the instance, ' +
94
+ 'but does not invoke them.', function() {
95
+ spyOn(console, 'warn');
96
+ var getInitialStateWasCalled = false;
97
+ var componentWillMountWasCalled = false;
98
+ class Foo extends React.Component {
99
+ constructor() {
100
+ this.contextTypes = {};
101
+ this.propTypes = {};
102
+ }
103
+ getInitialState() {
104
+ getInitialStateWasCalled = true;
105
+ return {};
106
+ }
107
+ componentWillMount() {
108
+ componentWillMountWasCalled = true;
109
+ }
110
+ render() {
111
+ return <span className="foo" />;
112
+ }
113
+ }
114
+ test(<Foo />, 'SPAN', 'foo');
115
+ // TODO: expect(getInitialStateWasCalled).toBe(false);
116
+ // TODO: expect(componentWillMountWasCalled).toBe(false);
117
+ expect(console.warn.calls.length).toBe(4);
118
+ expect(console.warn.calls[0].args[0]).toContain(
119
+ 'getInitialState was defined on Foo, a plain JavaScript class.'
120
+ );
121
+ expect(console.warn.calls[1].args[0]).toContain(
122
+ 'componentWillMount was defined on Foo, a plain JavaScript class.'
123
+ );
124
+ expect(console.warn.calls[2].args[0]).toContain(
125
+ 'propTypes was defined as an instance property on Foo.'
126
+ );
127
+ expect(console.warn.calls[3].args[0]).toContain(
128
+ 'contextTypes was defined as an instance property on Foo.'
129
+ );
130
+ });
131
+
132
+ it('should warn when mispelling shouldComponentUpdate', function() {
133
+ spyOn(console, 'warn');
134
+
135
+ class NamedComponent {
136
+ componentShouldUpdate() {
137
+ return false;
138
+ }
139
+ render() {
140
+ return <span className="foo" />;
141
+ }
142
+ }
143
+ test(<NamedComponent />, 'SPAN', 'foo');
144
+
145
+ expect(console.warn.calls.length).toBe(1);
146
+ expect(console.warn.calls[0].args[0]).toBe(
147
+ 'Warning: ' +
148
+ 'NamedComponent has a method called componentShouldUpdate(). Did you ' +
149
+ 'mean shouldComponentUpdate()? The name is phrased as a question ' +
150
+ 'because the function is expected to return a value.'
151
+ );
152
+ });
153
+
154
it('should throw AND warn when trying to access classic APIs', function() {
155
spyOn(console, 'warn');
156
var instance = test(<Inner name="foo" />, 'DIV', 'foo');