Merge pull request #2539 from sebmarkbage/cleanupinternals

Clean up a bunch of internal methods and fields that are now unnecessary

src/browser/ui/ReactDOMComponent.js

@@ -173,8 +173,8 @@ ReactDOMComponent.Mixin = {
         transaction,
         mountDepth
       );
-      assertValidProps(this.props);
       this._previousStyleCopy = null;
+      assertValidProps(this._currentElement.props);
       var closeTag = omittedCloseTags[this._tag] ? '' : '</' + this._tag + '>';
       return (
         this._createOpenTagMarkupAndPutListeners(transaction) +
@@ -197,7 +197,7 @@ ReactDOMComponent.Mixin = {
    * @return {string} Markup of opening tag.
    */
   _createOpenTagMarkupAndPutListeners: function(transaction) {
-    var props = this.props;
+    var props = this._currentElement.props;
     var ret = '<' + this._tag;
 
     for (var propKey in props) {
@@ -253,16 +253,18 @@ ReactDOMComponent.Mixin = {
       prefix = '\n';
     }
 
+    var props = this._currentElement.props;
+
     // Intentional use of != to avoid catching zero/false.
-    var innerHTML = this.props.dangerouslySetInnerHTML;
+    var innerHTML = props.dangerouslySetInnerHTML;
     if (innerHTML != null) {
       if (innerHTML.__html != null) {
         return prefix + innerHTML.__html;
       }
     } else {
       var contentToUse =
-        CONTENT_TYPES[typeof this.props.children] ? this.props.children : null;
-      var childrenToUse = contentToUse != null ? null : this.props.children;
+        CONTENT_TYPES[typeof props.children] ? props.children : null;
+      var childrenToUse = contentToUse != null ? null : props.children;
       if (contentToUse != null) {
         return prefix + escapeTextForBrowser(contentToUse);
       } else if (childrenToUse != null) {
@@ -338,7 +340,7 @@ ReactDOMComponent.Mixin = {
    * @param {ReactReconcileTransaction} transaction
    */
   _updateDOMProperties: function(lastProps, transaction) {
-    var nextProps = this.props;
+    var nextProps = this._currentElement.props;
     var propKey;
     var styleName;
     var styleUpdates;
@@ -427,7 +429,7 @@ ReactDOMComponent.Mixin = {
    * @param {ReactReconcileTransaction} transaction
    */
   _updateDOMChildren: function(lastProps, transaction) {
-    var nextProps = this.props;
+    var nextProps = this._currentElement.props;
 
     var lastContent =
       CONTENT_TYPES[typeof lastProps.children] ? lastProps.children : null;

src/browser/ui/__tests__/ReactDOMComponent-test.js

@@ -226,7 +226,7 @@ describe('ReactDOMComponent', function() {
       var ReactReconcileTransaction = require('ReactReconcileTransaction');
 
       var NodeStub = function(initialProps) {
-        this.props = initialProps || {};
+        this._currentElement = { props: initialProps };
         this._rootNodeID = 'test';
       };
       assign(NodeStub.prototype, ReactDOMComponent.Mixin);
@@ -276,7 +276,7 @@ describe('ReactDOMComponent', function() {
       var ReactReconcileTransaction = require('ReactReconcileTransaction');
 
       var NodeStub = function(initialProps) {
-        this.props = initialProps || {};
+        this._currentElement = { props: initialProps };
         this._rootNodeID = 'test';
       };
       assign(NodeStub.prototype, ReactDOMComponent.Mixin);

src/core/ReactComponent.js

@@ -20,21 +20,6 @@ var assign = require('Object.assign');
 var invariant = require('invariant');
 var keyMirror = require('keyMirror');
 
-/**
- * Every React component is in one of these life cycles.
- */
-var ComponentLifeCycle = keyMirror({
-  /**
-   * Mounted components have a DOM node representation and are capable of
-   * receiving new props.
-   */
-  MOUNTED: null,
-  /**
-   * Unmounted components are inactive and cannot receive new props.
-   */
-  UNMOUNTED: null
-});
-
 var injected = false;
 
 /**
@@ -115,11 +100,6 @@ var ReactComponent = {
     }
   },
 
-  /**
-   * @internal
-   */
-  LifeCycle: ComponentLifeCycle,
-
   /**
    * Injected module that provides ability to mutate individual properties.
    * Injected into the base class because many different subclasses need access
@@ -137,17 +117,6 @@ var ReactComponent = {
    */
   Mixin: {
 
-    /**
-     * Checks whether or not this component is mounted.
-     *
-     * @return {boolean} True if mounted, false otherwise.
-     * @final
-     * @protected
-     */
-    isMounted: function() {
-      return this._lifeCycleState === ComponentLifeCycle.MOUNTED;
-    },
-
     /**
      * Sets a subset of the props.
      *
@@ -175,10 +144,6 @@ var ReactComponent = {
      * @public
      */
     replaceProps: function(props, callback) {
-      invariant(
-        this.isMounted(),
-        'replaceProps(...): Can only update a mounted component.'
-      );
       invariant(
         this._mountDepth === 0,
         'replaceProps(...): You called `setProps` or `replaceProps` on a ' +
@@ -225,19 +190,6 @@ var ReactComponent = {
      * @internal
      */
     construct: function(element) {
-      // This is the public exposed props object after it has been processed
-      // with default props. The element's props represents the true internal
-      // state of the props.
-      this.props = element.props;
-      // Record the component responsible for creating this component.
-      // This is accessible through the element but we maintain an extra
-      // field for compatibility with devtools and as a way to make an
-      // incremental update. TODO: Consider deprecating this field.
-      this._owner = element._owner;
-
-      // All components start unmounted.
-      this._lifeCycleState = ComponentLifeCycle.UNMOUNTED;
-
       // See ReactUpdates.
       this._pendingCallbacks = null;
 
@@ -262,20 +214,12 @@ var ReactComponent = {
      * @internal
      */
     mountComponent: function(rootID, transaction, mountDepth) {
-      invariant(
-        !this.isMounted(),
-        'mountComponent(%s, ...): Can only mount an unmounted component. ' +
-        'Make sure to avoid storing components between renders or reusing a ' +
-        'single component instance in multiple places.',
-        rootID
-      );
       var ref = this._currentElement.ref;
       if (ref != null) {
         var owner = this._currentElement._owner;
         attachRef(ref, this, owner);
       }
       this._rootNodeID = rootID;
-      this._lifeCycleState = ComponentLifeCycle.MOUNTED;
       this._mountDepth = mountDepth;
       // Effectively: return '';
     },
@@ -291,17 +235,15 @@ var ReactComponent = {
      * @internal
      */
     unmountComponent: function() {
-      invariant(
-        this.isMounted(),
-        'unmountComponent(): Can only unmount a mounted component.'
-      );
       var ref = this._currentElement.ref;
       if (ref != null) {
-        detachRef(ref, this, this._owner);
+        detachRef(ref, this, this._currentElement._owner);
       }
       unmountIDFromEnvironment(this._rootNodeID);
+      // Reset all fields
       this._rootNodeID = null;
-      this._lifeCycleState = ComponentLifeCycle.UNMOUNTED;
+      this._pendingCallbacks = null;
+      this._pendingElement = null;
     },
 
     /**
@@ -316,10 +258,6 @@ var ReactComponent = {
      * @internal
      */
     receiveComponent: function(nextElement, transaction) {
-      invariant(
-        this.isMounted(),
-        'receiveComponent(...): Can only update a mounted component.'
-      );
       this._pendingElement = nextElement;
       this.performUpdateIfNecessary(transaction);
     },
@@ -337,8 +275,6 @@ var ReactComponent = {
       var prevElement = this._currentElement;
       var nextElement = this._pendingElement;
       this._currentElement = nextElement;
-      this.props = nextElement.props;
-      this._owner = nextElement._owner;
       this._pendingElement = null;
       this.updateComponent(transaction, prevElement, nextElement);
     },
@@ -416,34 +352,6 @@ var ReactComponent = {
       mountImageIntoNode(markup, container, shouldReuseMarkup);
     },
 
-    /**
-     * Checks if this component is owned by the supplied `owner` component.
-     *
-     * @param {ReactComponent} owner Component to check.
-     * @return {boolean} True if `owners` owns this component.
-     * @final
-     * @internal
-     */
-    isOwnedBy: function(owner) {
-      return this._owner === owner;
-    },
-
-    /**
-     * Gets another component, that shares the same owner as this one, by ref.
-     *
-     * @param {string} ref of a sibling Component.
-     * @return {?ReactComponent} the actual sibling Component.
-     * @final
-     * @internal
-     */
-    getSiblingByRef: function(ref) {
-      var owner = this._owner;
-      if (!owner || !owner.refs) {
-        return null;
-      }
-      return owner.refs[ref];
-    },
-
     /**
      * Get the publicly accessible representation of this component - i.e. what
      * is exposed by refs and renderComponent. Can be null for stateless

src/core/ReactCompositeComponent.js

@@ -16,19 +16,19 @@ var ReactContext = require('ReactContext');
 var ReactCurrentOwner = require('ReactCurrentOwner');
 var ReactElement = require('ReactElement');
 var ReactInstanceMap = require('ReactInstanceMap');
-var ReactOwner = require('ReactOwner');
 var ReactPerf = require('ReactPerf');
 var ReactPropTypeLocations = require('ReactPropTypeLocations');
 var ReactUpdates = require('ReactUpdates');
 
 var assign = require('Object.assign');
+var emptyObject = require('emptyObject');
 var invariant = require('invariant');
 var keyMirror = require('keyMirror');
 var shouldUpdateReactComponent = require('shouldUpdateReactComponent');
 var warning = require('warning');
 
 function getDeclarationErrorAddendum(component) {
-  var owner = component._owner || null;
+  var owner = component._currentElement._owner || null;
   if (owner) {
     var constructor = owner._instance.constructor;
     if (constructor && constructor.displayName) {
@@ -56,8 +56,8 @@ function validateLifeCycleOnReplaceState(instance) {
  * `ReactCompositeComponent` maintains an auxiliary life cycle state in
  * `this._compositeLifeCycleState` (which can be null).
  *
- * This is different from the life cycle state maintained by `ReactComponent` in
- * `this._lifeCycleState`. The following diagram shows how the states overlap in
+ * This is different from the life cycle state maintained by `ReactComponent`.
+ * The following diagram shows how the states overlap in
  * time. There are times when the CompositeLifeCycle is null - at those times it
  * is only meaningful to look at ComponentLifeCycle alone.
  *
@@ -100,8 +100,7 @@ var CompositeLifeCycle = keyMirror({
  * @lends {ReactCompositeComponent.prototype}
  */
 var ReactCompositeComponentMixin = assign({},
-  ReactComponent.Mixin,
-  ReactOwner.Mixin, {
+  ReactComponent.Mixin, {
 
   /**
    * Base constructor for all composite component.
@@ -114,13 +113,13 @@ var ReactCompositeComponentMixin = assign({},
     this._instance.props = element.props;
     this._instance.state = null;
     this._instance.context = null;
+    this._instance.refs = emptyObject;
 
     this._pendingState = null;
     this._compositeLifeCycleState = null;
 
     // Children can be either an array or more than one argument
     ReactComponent.Mixin.construct.apply(this, arguments);
-    ReactOwner.Mixin.construct.apply(this, arguments);
   },
 
   /**
@@ -130,8 +129,7 @@ var ReactCompositeComponentMixin = assign({},
    * @final
    */
   isMounted: function() {
-    return ReactComponent.Mixin.isMounted.call(this) &&
-      this._compositeLifeCycleState !== CompositeLifeCycle.MOUNTING;
+    return this._compositeLifeCycleState !== CompositeLifeCycle.MOUNTING;
   },
 
   /**
@@ -233,6 +231,9 @@ var ReactCompositeComponentMixin = assign({},
     this._renderedComponent.unmountComponent();
     this._renderedComponent = null;
 
+    // Reset pending fields
+    this._pendingState = null;
+    this._pendingForceUpdate = false;
     ReactComponent.Mixin.unmountComponent.call(this);
 
     // Delete the reference from the instance to this internal representation
@@ -555,11 +556,6 @@ var ReactCompositeComponentMixin = assign({},
         inst.props = nextProps;
         inst.state = nextState;
         inst.context = nextContext;
-
-        // Owner cannot change because shouldUpdateReactComponent doesn't allow
-        // it. TODO: Remove this._owner completely.
-        this._owner = nextParentElement._owner;
-
         return;
       }
 
@@ -607,10 +603,6 @@ var ReactCompositeComponentMixin = assign({},
     inst.state = nextState;
     inst.context = nextContext;
 
-    // Owner cannot change because shouldUpdateReactComponent doesn't allow
-    // it. TODO: Remove this._owner completely.
-    this._owner = nextElement._owner;
-
     this._updateRenderedComponent(transaction);
 
     if (inst.componentDidUpdate) {
@@ -699,6 +691,32 @@ var ReactCompositeComponentMixin = assign({},
     }
   ),
 
+  /**
+   * Lazily allocates the refs object and stores `component` as `ref`.
+   *
+   * @param {string} ref Reference name.
+   * @param {component} component Component to store as `ref`.
+   * @final
+   * @private
+   */
+  attachRef: function(ref, component) {
+    var inst = this.getPublicInstance();
+    var refs = inst.refs === emptyObject ? (inst.refs = {}) : inst.refs;
+    refs[ref] = component.getPublicInstance();
+  },
+
+  /**
+   * Detaches a reference name.
+   *
+   * @param {string} ref Name to dereference.
+   * @final
+   * @private
+   */
+  detachRef: function(ref) {
+    var refs = this.getPublicInstance().refs;
+    delete refs[ref];
+  },
+
   /**
    * Get the publicly accessible representation of this component - i.e. what
    * is exposed by refs and renderComponent. Can be null for stateless

src/core/ReactElementValidator.js

@@ -160,7 +160,7 @@ function monitorUseOfObjectMap() {
 }
 
 /**
- * Ensure that every component either is passed in a static location, in an
+ * Ensure that every element either is passed in a static location, in an
  * array with an explicit keys property defined, or in an object literal
  * with valid key property.
  *

src/core/ReactOwner.js

@@ -11,7 +11,6 @@
 
 "use strict";
 
-var emptyObject = require('emptyObject');
 var invariant = require('invariant');
 
 /**
@@ -103,53 +102,6 @@ var ReactOwner = {
     if (owner.getPublicInstance().refs[ref] === component.getPublicInstance()) {
       owner.detachRef(ref);
     }
-  },
-
-  /**
-   * A ReactComponent must mix this in to have refs.
-   *
-   * @lends {ReactOwner.prototype}
-   */
-  Mixin: {
-
-    construct: function() {
-      var inst = this.getPublicInstance();
-      inst.refs = emptyObject;
-    },
-
-    /**
-     * Lazily allocates the refs object and stores `component` as `ref`.
-     *
-     * @param {string} ref Reference name.
-     * @param {component} component Component to store as `ref`.
-     * @final
-     * @private
-     */
-    attachRef: function(ref, component) {
-      // TODO: Remove this invariant. This is never exposed and cannot be called
-      // by user code. The unit test is already removed.
-      invariant(
-        component.isOwnedBy(this),
-        'attachRef(%s, ...): Only a component\'s owner can store a ref to it.',
-        ref
-      );
-      var inst = this.getPublicInstance();
-      var refs = inst.refs === emptyObject ? (inst.refs = {}) : inst.refs;
-      refs[ref] = component.getPublicInstance();
-    },
-
-    /**
-     * Detaches a reference name.
-     *
-     * @param {string} ref Name to dereference.
-     * @final
-     * @private
-     */
-    detachRef: function(ref) {
-      var refs = this.getPublicInstance().refs;
-      delete refs[ref];
-    }
-
   }
 
 };

src/core/ReactUpdates.js

@@ -136,24 +136,23 @@ function runBatchedUpdates(transaction) {
   dirtyComponents.sort(mountDepthComparator);
 
   for (var i = 0; i < len; i++) {
-    // If a component is unmounted before pending changes apply, ignore them
-    // TODO: Queue unmounts in the same list to avoid this happening at all
+    // If a component is unmounted before pending changes apply, it will still
+    // be here, but we assume that it has cleared its _pendingCallbacks and
+    // that performUpdateIfNecessary is a noop.
     var component = dirtyComponents[i];
-    if (component.isMounted()) {
-      // If performUpdateIfNecessary happens to enqueue any new updates, we
-      // shouldn't execute the callbacks until the next render happens, so
-      // stash the callbacks first
-      var callbacks = component._pendingCallbacks;
-      component._pendingCallbacks = null;
-      component.performUpdateIfNecessary(transaction.reconcileTransaction);
+    // If performUpdateIfNecessary happens to enqueue any new updates, we
+    // shouldn't execute the callbacks until the next render happens, so
+    // stash the callbacks first
+    var callbacks = component._pendingCallbacks;
+    component._pendingCallbacks = null;
+    component.performUpdateIfNecessary(transaction.reconcileTransaction);
 
-      if (callbacks) {
-        for (var j = 0; j < callbacks.length; j++) {
-          transaction.callbackQueue.enqueue(
-            callbacks[j],
-            component
-          );
-        }
+    if (callbacks) {
+      for (var j = 0; j < callbacks.length; j++) {
+        transaction.callbackQueue.enqueue(
+          callbacks[j],
+          component
+        );
       }
     }
   }

src/core/__tests__/ReactComponentLifeCycle-test.js

@@ -11,12 +11,13 @@
 
 "use strict";
 
+var keyMirror = require('keyMirror');
+
 var React;
 var ReactTestUtils;
 var ReactComponent;
 var ReactCompositeComponent;
 var ReactInstanceMap;
-var ComponentLifeCycle;
 var CompositeComponentLifeCycle;
 
 var getCompositeLifeCycle;
@@ -69,6 +70,21 @@ var POST_WILL_UNMOUNT_STATE = {
   hasWillUnmountCompleted: true
 };
 
+/**
+ * Every React component is in one of these life cycles.
+ */
+var ComponentLifeCycle = keyMirror({
+  /**
+   * Mounted components have a DOM node representation and are capable of
+   * receiving new props.
+   */
+  MOUNTED: null,
+  /**
+   * Unmounted components are inactive and cannot receive new props.
+   */
+  UNMOUNTED: null
+});
+
 /**
  * TODO: We should make any setState calls fail in
  * `getInitialState` and `componentWillMount`. They will usually fail
@@ -83,7 +99,6 @@ describe('ReactComponentLifeCycle', function() {
     ReactTestUtils = require('ReactTestUtils');
     ReactComponent = require('ReactComponent');
     ReactCompositeComponent = require('ReactCompositeComponent');
-    ComponentLifeCycle = ReactComponent.LifeCycle;
     CompositeComponentLifeCycle = ReactCompositeComponent.LifeCycle;
 
     ReactInstanceMap = require('ReactInstanceMap');
@@ -94,13 +109,11 @@ describe('ReactComponentLifeCycle', function() {
 
     getLifeCycleState = function(instance) {
       var internalInstance = ReactInstanceMap.get(instance);
-      if (!internalInstance) {
-        // Once a component is fully unmounted, we cannot actually get to the
-        // internal instance. It's already dereferenced and possibly GC:ed.
-        // So the unmounted life cycle hook doesn't exist anymore.
-        return ComponentLifeCycle.UNMOUNTED;
-      }
-      return internalInstance._lifeCycleState;
+      // Once a component gets mounted, it has an internal instance, once it
+      // gets unmounted, it loses that internal instance.
+      return internalInstance ?
+             ComponentLifeCycle.MOUNTED :
+             ComponentLifeCycle.UNMOUNTED;
     };
   });
 

src/core/__tests__/ReactElement-test.js

@@ -173,6 +173,36 @@ describe('ReactElement', function() {
     );
   });
 
+  it('warns for keys for arrays of elements in rest args', function() {
+    spyOn(console, 'warn');
+    var Component = React.createFactory(ComponentClass);
+
+    var InnerClass = React.createClass({
+      displayName: 'InnerClass',
+      render: function() {
+        return Component(null, this.props.childSet);
+      }
+    });
+
+    var InnerComponent = React.createFactory(InnerClass);
+
+    var ComponentWrapper = React.createClass({
+      displayName: 'ComponentWrapper',
+      render: function() {
+        return InnerComponent({ childSet: [ Component(), Component() ] });
+      }
+    });
+
+    ReactTestUtils.renderIntoDocument(<ComponentWrapper />);
+
+    expect(console.warn.argsForCall.length).toBe(1);
+    expect(console.warn.argsForCall[0][0]).toContain(
+      'Each child in an array or iterator should have a unique "key" prop. ' +
+      'Check the render method of InnerClass. ' +
+      'It was passed a child from ComponentWrapper. '
+    );
+  });
+
   it('warns for keys for iterables of elements in rest args', function() {
     spyOn(console, 'warn');
     var Component = React.createFactory(ComponentClass);

src/core/__tests__/ReactUpdates-test.js

@@ -815,4 +815,46 @@ describe('ReactUpdates', function() {
       'asap-1.2'
     ]);
   });
+
+  it('does not call render after a component as been deleted', function() {
+
+    var renderCount = 0;
+    var componentB = null;
+
+    var B = React.createClass({
+      getInitialState: function() {
+        return { updates: 0 };
+      },
+      componentDidMount: function() {
+        componentB = this;
+      },
+      render: function() {
+        renderCount++;
+        return <div />;
+      }
+    });
+
+    var A = React.createClass({
+      getInitialState: function() {
+        return { showB: true };
+      },
+      render: function() {
+        return this.state.showB ? <B /> : <div />;
+      }
+    });
+
+    var container = document.createElement('div');
+
+    var component = React.render(<A />, container);
+
+    ReactUpdates.batchedUpdates(function() {
+      // B will have scheduled an update but the batching should ensure that its
+      // update never fires.
+      componentB.setState({ updates: 1 });
+      component.setState({ showB: false });
+    });
+
+    expect(renderCount).toBe(1);
+  });
+
 });

src/test/ReactDefaultPerf.js

@@ -244,7 +244,8 @@ var ReactDefaultPerf = {
 
         entry.displayNames[rootNodeID] = {
           current: this.constructor.displayName,
-          owner: this._owner ? this._owner.constructor.displayName : '<root>'
+          owner: this._currentElement._owner ?
+                 this._currentElement._owner.constructor.displayName : '<root>'
         };
 
         return rv;