Add warnings when accessing properties/methods on unmounted components

This creates a membrane around the React component prototype. It warns if you
try to access properties on the component before it's unmounted. Before it's
mounted, it should be considered a descriptor and not an actual instance.

The workaround, for unknown types, is to access the constructor using
component.type which has static methods on it.
This commit is contained in:
Sebastian Markbage
2014-02-11 09:13:03 -08:00
committed by Paul O’Shannessy
parent 0f4cc6ee84
commit fc2805fe03
7 changed files with 221 additions and 19 deletions
+6
View File
@@ -51,6 +51,12 @@ function createDOMComponentClass(tag, omitClose) {
return instance;
};
// Expose the constructor on the ConvenienceConstructor and prototype so that
// it can be easily easily accessed on descriptors.
// E.g. <div />.type === div.type
ConvenienceConstructor.type = Constructor;
Constructor.prototype.type = Constructor;
Constructor.ConvenienceConstructor = ConvenienceConstructor;
ConvenienceConstructor.componentConstructor = Constructor;
return ConvenienceConstructor;
+5
View File
@@ -91,4 +91,9 @@ mixInto(ReactTextComponent, {
});
// Expose the constructor on itself and the prototype for consistency with other
// descriptors.
ReactTextComponent.type = ReactTextComponent;
ReactTextComponent.prototype.type = ReactTextComponent;
module.exports = ReactTextComponent;
+11 -4
View File
@@ -183,10 +183,17 @@ var ReactComponent = {
* @final
*/
isValidComponent: function(object) {
return !!(
object &&
typeof object.mountComponentIntoNode === 'function' &&
typeof object.receiveComponent === 'function'
if (!object || !object.type || !object.type.prototype) {
return false;
}
// This is the safer way of duck checking the type of instance this is.
// The object can be a generic descriptor but the type property refers to
// the constructor and it's prototype can be used to inspect the type that
// will actually get mounted.
var prototype = object.type.prototype;
return (
typeof prototype.mountComponentIntoNode === 'function' &&
typeof prototype.receiveComponent === 'function'
);
},
+124
View File
@@ -584,6 +584,118 @@ function createChainedFunction(one, two) {
};
}
if (__DEV__) {
var unmountedPropertyWhitelist = {
constructor: true,
construct: true,
isOwnedBy: true, // should be deprecated but can have code mod (internal)
mountComponent: true,
mountComponentIntoNode: true,
props: true,
type: true,
_checkPropTypes: true,
_mountComponentIntoNode: true,
_processContext: true
};
var hasWarnedOnComponentType = {};
var warnIfUnmounted = function(instance, key) {
if (instance.__hasBeenMounted) {
return;
}
var name = instance.constructor.displayName || 'Unknown';
var owner = ReactCurrentOwner.current;
var ownerName = (owner && owner.constructor.displayName) || 'Unknown';
var warningKey = key + '|' + name + '|' + ownerName;
if (hasWarnedOnComponentType.hasOwnProperty(warningKey)) {
// We have already warned for this combination. Skip it this time.
return;
}
hasWarnedOnComponentType[warningKey] = true;
var context = owner ? ' in ' + ownerName + '.' : ' at the top level.';
var staticMethodExample = '<' + name + ' />.type.' + key + '(...)';
console.warn(
'Invalid access to component property "' + key + '" on ' + name +
context + ' See http://fb.me/react-warning-descriptors .' +
' Use a static method instead: ' + staticMethodExample
);
};
var defineMembraneProperty = function(membrane, prototype, key) {
Object.defineProperty(membrane, key, {
configurable: false,
enumerable: true,
get: function() {
if (this !== membrane) {
// When this is accessed through a prototype chain we need to check if
// this component was mounted.
warnIfUnmounted(this, key);
}
return prototype[key];
},
set: function(value) {
if (this !== membrane) {
// When this is accessed through a prototype chain, we first check if
// this component was mounted. Then we define a value on "this"
// instance, effectively disabling the membrane on that prototype
// chain.
warnIfUnmounted(this, key);
Object.defineProperty(this, key, {
enumerable: true,
configurable: true,
writable: true,
value: value
});
} else {
// Otherwise, this should modify the prototype
prototype[key] = value;
}
}
});
};
/**
* Creates a membrane prototype which wraps the original prototype. If any
* property is accessed in an unmounted state, a warning is issued.
*
* @param {object} prototype Original prototype.
* @return {object} The membrane prototype.
* @private
*/
var createMountWarningMembrane = function(prototype) {
try {
var membrane = Object.create(prototype);
for (var key in prototype) {
if (unmountedPropertyWhitelist.hasOwnProperty(key)) {
continue;
}
defineMembraneProperty(membrane, prototype, key);
}
membrane.mountComponent = function() {
this.__hasBeenMounted = true;
return prototype.mountComponent.apply(this, arguments);
};
return membrane;
} catch(x) {
// In IE8 define property will fail on non-DOM objects. If anything in
// the membrane creation fails, we'll bail out and just use the prototype
// without warnings.
return prototype;
}
};
}
/**
* `ReactCompositeComponent` maintains an auxiliary life cycle state in
* `this._compositeLifeCycleState` (which can be null).
@@ -1307,6 +1419,14 @@ var ReactCompositeComponent = {
}
}
// Expose the convience constructor on the prototype so that it can be
// easily accessed on descriptors. E.g. <Foo />.type === Foo.type and for
// static methods like <Foo />.type.staticMethod();
// This should not be named constructor since this may not be the function
// that created the descriptor, and it may not even be a constructor.
ConvenienceConstructor.type = Constructor;
Constructor.prototype.type = Constructor;
// Reduce time spent doing lookups by setting these on the prototype.
for (var methodName in ReactCompositeComponentInterface) {
if (!Constructor.prototype[methodName]) {
@@ -1314,6 +1434,10 @@ var ReactCompositeComponent = {
}
}
if (__DEV__) {
Constructor.prototype = createMountWarningMembrane(Constructor.prototype);
}
return ConvenienceConstructor;
},
+2 -3
View File
@@ -126,10 +126,9 @@ describe('ReactComponent', function() {
}
});
var instance = <Component />;
var descriptor = <Component />;
expect(instance.isMounted()).toBeFalsy();
ReactTestUtils.renderIntoDocument(instance);
var instance = ReactTestUtils.renderIntoDocument(descriptor);
expect(instance.isMounted()).toBeTruthy();
});
@@ -170,6 +170,7 @@ describe('ReactCompositeComponent', function() {
// These are controversial assertions for now, they just exist
// because existing code depends on these assumptions.
// These are expected to log a warning. This use case will be deprecated.
expect(function() {
instance.methodToBeExplicitlyBound.bind(instance)();
}).not.toThrow();
@@ -183,9 +184,9 @@ describe('ReactCompositeComponent', function() {
// Next, prove that once mounted, the scope is bound correctly to the actual
// component.
ReactTestUtils.renderIntoDocument(instance);
expect(console.warn.argsForCall.length).toBe(0);
expect(console.warn.argsForCall.length).toBe(3);
var explicitlyBound = instance.methodToBeExplicitlyBound.bind(instance);
expect(console.warn.argsForCall.length).toBe(1);
expect(console.warn.argsForCall.length).toBe(4);
var autoBound = instance.methodAutoBound;
var explicitlyNotBound = instance.methodExplicitlyNotBound;
@@ -1105,4 +1106,61 @@ describe('ReactCompositeComponent', function() {
'use a component class as a mixin. Instead, just use a regular object.'
);
});
it('should warn if an umounted component is touched', function() {
spyOn(console, 'warn');
var ComponentClass = React.createClass({
getInitialState: function() {
return {valueToReturn: 'hi'};
},
someMethod: function() {
return this;
},
someOtherMethod: function() {
return this;
},
render: function() {
return <div></div>;
}
});
var descriptor = <ComponentClass />;
var instance = ReactTestUtils.renderIntoDocument(descriptor);
instance.someMethod();
expect(console.warn.argsForCall.length).toBe(0);
var unmountedInstance = <ComponentClass />;
var result = unmountedInstance.someMethod();
expect(console.warn.argsForCall.length).toBe(1);
expect(result).toBe(unmountedInstance);
var unmountedInstance2 = <ComponentClass />;
unmountedInstance2.someOtherMethod = 'override';
expect(console.warn.argsForCall.length).toBe(2);
expect(unmountedInstance2.someOtherMethod).toBe('override');
});
it('should allow static methods called using type property', function() {
spyOn(console, 'warn');
var ComponentClass = React.createClass({
statics: {
someStaticMethod: function() {
return 'someReturnValue';
}
},
getInitialState: function() {
return {valueToReturn: 'hi'};
},
render: function() {
return <div></div>;
}
});
var descriptor = <ComponentClass />;
expect(descriptor.type.someStaticMethod()).toBe('someReturnValue');
expect(console.warn.argsForCall.length).toBe(0);
});
});
+13 -10
View File
@@ -51,11 +51,10 @@ var ReactTestUtils = {
return React.renderComponent(instance, div);
},
isComponentOfType: function(inst, type) {
return !!(
inst &&
isComponentOfType: function(inst, convenienceConstructor) {
return (
ReactComponent.isValidComponent(inst) &&
inst.constructor === type.componentConstructor
inst.type === convenienceConstructor.type
);
},
@@ -66,12 +65,16 @@ var ReactTestUtils = {
},
isCompositeComponent: function(inst) {
return !!(
inst &&
ReactComponent.isValidComponent(inst) &&
typeof inst.render === 'function' &&
typeof inst.setState === 'function' &&
typeof inst.updateComponent === 'function'
if (!ReactComponent.isValidComponent(inst)) {
return false;
}
// We check the prototype of the type that will get mounted, not the
// instance itself. This is a future proof way of duck typing.
var prototype = inst.type.prototype;
return (
typeof prototype.render === 'function' &&
typeof prototype.setState === 'function' &&
typeof prototype.updateComponent === 'function'
);
},