From fc2805fe03889a8033bb5f651e7b6937b45d33df Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Tue, 11 Feb 2014 09:00:14 -0800 Subject: [PATCH] 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. --- src/browser/ReactDOM.js | 6 + src/browser/ReactTextComponent.js | 5 + src/core/ReactComponent.js | 15 ++- src/core/ReactCompositeComponent.js | 124 ++++++++++++++++++ src/core/__tests__/ReactComponent-test.js | 5 +- .../__tests__/ReactCompositeComponent-test.js | 62 ++++++++- src/test/ReactTestUtils.js | 23 ++-- 7 files changed, 221 insertions(+), 19 deletions(-) diff --git a/src/browser/ReactDOM.js b/src/browser/ReactDOM.js index abe0630302..b7eeecf004 100644 --- a/src/browser/ReactDOM.js +++ b/src/browser/ReactDOM.js @@ -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.
.type === div.type + ConvenienceConstructor.type = Constructor; + Constructor.prototype.type = Constructor; + Constructor.ConvenienceConstructor = ConvenienceConstructor; ConvenienceConstructor.componentConstructor = Constructor; return ConvenienceConstructor; diff --git a/src/browser/ReactTextComponent.js b/src/browser/ReactTextComponent.js index 79114b9e7b..63a69ec1c1 100644 --- a/src/browser/ReactTextComponent.js +++ b/src/browser/ReactTextComponent.js @@ -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; diff --git a/src/core/ReactComponent.js b/src/core/ReactComponent.js index 64f9d7652d..49de894323 100644 --- a/src/core/ReactComponent.js +++ b/src/core/ReactComponent.js @@ -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' ); }, diff --git a/src/core/ReactCompositeComponent.js b/src/core/ReactCompositeComponent.js index 63b1e8358d..b8598e9b00 100644 --- a/src/core/ReactCompositeComponent.js +++ b/src/core/ReactCompositeComponent.js @@ -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. .type === Foo.type and for + // static methods like .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; }, diff --git a/src/core/__tests__/ReactComponent-test.js b/src/core/__tests__/ReactComponent-test.js index 7b045c1dcc..2b4849fe9b 100644 --- a/src/core/__tests__/ReactComponent-test.js +++ b/src/core/__tests__/ReactComponent-test.js @@ -126,10 +126,9 @@ describe('ReactComponent', function() { } }); - var instance = ; + var descriptor = ; - expect(instance.isMounted()).toBeFalsy(); - ReactTestUtils.renderIntoDocument(instance); + var instance = ReactTestUtils.renderIntoDocument(descriptor); expect(instance.isMounted()).toBeTruthy(); }); diff --git a/src/core/__tests__/ReactCompositeComponent-test.js b/src/core/__tests__/ReactCompositeComponent-test.js index fc4542b5af..c82b8efcc7 100644 --- a/src/core/__tests__/ReactCompositeComponent-test.js +++ b/src/core/__tests__/ReactCompositeComponent-test.js @@ -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
; + } + }); + + var descriptor = ; + var instance = ReactTestUtils.renderIntoDocument(descriptor); + instance.someMethod(); + expect(console.warn.argsForCall.length).toBe(0); + + var unmountedInstance = ; + var result = unmountedInstance.someMethod(); + expect(console.warn.argsForCall.length).toBe(1); + expect(result).toBe(unmountedInstance); + + var unmountedInstance2 = ; + 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
; + } + }); + + var descriptor = ; + expect(descriptor.type.someStaticMethod()).toBe('someReturnValue'); + expect(console.warn.argsForCall.length).toBe(0); + }); + }); diff --git a/src/test/ReactTestUtils.js b/src/test/ReactTestUtils.js index 16c0c1c42c..f54f7555d5 100644 --- a/src/test/ReactTestUtils.js +++ b/src/test/ReactTestUtils.js @@ -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' ); },