Merge pull request #4825 from spicyj/gh-2770

Preserve DOM node when updating empty component

src/renderers/dom/client/ReactMount.js

@@ -16,7 +16,7 @@ var ReactBrowserEventEmitter = require('ReactBrowserEventEmitter');
 var ReactCurrentOwner = require('ReactCurrentOwner');
 var ReactDOMFeatureFlags = require('ReactDOMFeatureFlags');
 var ReactElement = require('ReactElement');
-var ReactEmptyComponent = require('ReactEmptyComponent');
+var ReactEmptyComponentRegistry = require('ReactEmptyComponentRegistry');
 var ReactInstanceHandles = require('ReactInstanceHandles');
 var ReactInstanceMap = require('ReactInstanceMap');
 var ReactMarkupChecksum = require('ReactMarkupChecksum');
@@ -181,7 +181,7 @@ function getNode(id) {
  */
 function getNodeFromInstance(instance) {
   var id = ReactInstanceMap.get(instance)._rootNodeID;
-  if (ReactEmptyComponent.isNullComponentID(id)) {
+  if (ReactEmptyComponentRegistry.isNullComponentID(id)) {
     return null;
   }
   if (!nodeCache.hasOwnProperty(id) || !isValid(nodeCache[id], id)) {

src/renderers/shared/reconciler/ReactChildReconciler.js

@@ -90,7 +90,8 @@ var ReactChildReconciler = {
       var prevChild = prevChildren && prevChildren[name];
       var prevElement = prevChild && prevChild._currentElement;
       var nextElement = nextChildren[name];
-      if (shouldUpdateReactComponent(prevElement, nextElement)) {
+      if (prevChild != null &&
+          shouldUpdateReactComponent(prevElement, nextElement)) {
         ReactReconciler.receiveComponent(
           prevChild, nextElement, transaction, context
         );

src/renderers/shared/reconciler/ReactEmptyComponent.js

@@ -12,81 +12,47 @@
 'use strict';
 
 var ReactElement = require('ReactElement');
-var ReactInstanceMap = require('ReactInstanceMap');
+var ReactEmptyComponentRegistry = require('ReactEmptyComponentRegistry');
+var ReactReconciler = require('ReactReconciler');
 
-var invariant = require('invariant');
+var assign = require('Object.assign');
 
-var component;
-// This registry keeps track of the React IDs of the components that rendered to
-// `null` (in reality a placeholder such as `noscript`)
-var nullComponentIDsRegistry = {};
+var placeholderElement;
 
 var ReactEmptyComponentInjection = {
-  injectEmptyComponent: function(emptyComponent) {
-    component = ReactElement.createFactory(emptyComponent);
+  injectEmptyComponent: function(component) {
+    placeholderElement = ReactElement.createElement(component);
   },
 };
 
-var ReactEmptyComponentType = function() {};
-ReactEmptyComponentType.isReactClass = {};
-ReactEmptyComponentType.prototype.componentDidMount = function() {
-  var internalInstance = ReactInstanceMap.get(this);
-  // TODO: Make sure we run these methods in the correct order, we shouldn't
-  // need this check. We're going to assume if we're here it means we ran
-  // componentWillUnmount already so there is no internal instance (it gets
-  // removed as part of the unmounting process).
-  if (!internalInstance) {
-    return;
-  }
-  registerNullComponentID(internalInstance._rootNodeID);
-};
-ReactEmptyComponentType.prototype.componentWillUnmount = function() {
-  var internalInstance = ReactInstanceMap.get(this);
-  // TODO: Get rid of this check. See TODO in componentDidMount.
-  if (!internalInstance) {
-    return;
-  }
-  deregisterNullComponentID(internalInstance._rootNodeID);
-};
-ReactEmptyComponentType.prototype.render = function() {
-  invariant(
-    component,
-    'Trying to return null from a render, but no null placeholder component ' +
-    'was injected.'
-  );
-  return component();
+var ReactEmptyComponent = function(instantiate) {
+  this._currentElement = null;
+  this._rootNodeID = null;
+  this._renderedComponent = instantiate(placeholderElement);
 };
+assign(ReactEmptyComponent.prototype, {
+  construct: function(element) {
+  },
+  mountComponent: function(rootID, transaction, context) {
+    ReactEmptyComponentRegistry.registerNullComponentID(rootID);
+    this._rootNodeID = rootID;
+    return ReactReconciler.mountComponent(
+      this._renderedComponent,
+      rootID,
+      transaction,
+      context
+    );
+  },
+  receiveComponent: function() {
+  },
+  unmountComponent: function(rootID, transaction, context) {
+    ReactReconciler.unmountComponent(this._renderedComponent);
+    ReactEmptyComponentRegistry.deregisterNullComponentID(this._rootNodeID);
+    this._rootNodeID = null;
+    this._renderedComponent = null;
+  },
+});
 
-var emptyElement = ReactElement.createElement(ReactEmptyComponentType);
-
-/**
- * Mark the component as having rendered to null.
- * @param {string} id Component's `_rootNodeID`.
- */
-function registerNullComponentID(id) {
-  nullComponentIDsRegistry[id] = true;
-}
-
-/**
- * Unmark the component as having rendered to null: it renders to something now.
- * @param {string} id Component's `_rootNodeID`.
- */
-function deregisterNullComponentID(id) {
-  delete nullComponentIDsRegistry[id];
-}
-
-/**
- * @param {string} id Component's `_rootNodeID`.
- * @return {boolean} True if the component is rendered to null.
- */
-function isNullComponentID(id) {
-  return !!nullComponentIDsRegistry[id];
-}
-
-var ReactEmptyComponent = {
-  emptyElement: emptyElement,
-  injection: ReactEmptyComponentInjection,
-  isNullComponentID: isNullComponentID,
-};
+ReactEmptyComponent.injection = ReactEmptyComponentInjection;
 
 module.exports = ReactEmptyComponent;

src/renderers/shared/reconciler/ReactEmptyComponentRegistry.js

@@ -0,0 +1,48 @@
+/**
+ * Copyright 2014-2015, Facebook, Inc.
+ * All rights reserved.
+ *
+ * This source code is licensed under the BSD-style license found in the
+ * LICENSE file in the root directory of this source tree. An additional grant
+ * of patent rights can be found in the PATENTS file in the same directory.
+ *
+ * @providesModule ReactEmptyComponentRegistry
+ */
+
+'use strict';
+
+// This registry keeps track of the React IDs of the components that rendered to
+// `null` (in reality a placeholder such as `noscript`)
+var nullComponentIDsRegistry = {};
+
+/**
+ * @param {string} id Component's `_rootNodeID`.
+ * @return {boolean} True if the component is rendered to null.
+ */
+function isNullComponentID(id) {
+  return !!nullComponentIDsRegistry[id];
+}
+
+/**
+ * Mark the component as having rendered to null.
+ * @param {string} id Component's `_rootNodeID`.
+ */
+function registerNullComponentID(id) {
+  nullComponentIDsRegistry[id] = true;
+}
+
+/**
+ * Unmark the component as having rendered to null: it renders to something now.
+ * @param {string} id Component's `_rootNodeID`.
+ */
+function deregisterNullComponentID(id) {
+  delete nullComponentIDsRegistry[id];
+}
+
+var ReactEmptyComponentRegistry = {
+  isNullComponentID: isNullComponentID,
+  registerNullComponentID: registerNullComponentID,
+  deregisterNullComponentID: deregisterNullComponentID,
+};
+
+module.exports = ReactEmptyComponentRegistry;

src/renderers/shared/reconciler/ReactReconciler.js

@@ -35,7 +35,8 @@ var ReactReconciler = {
    */
   mountComponent: function(internalInstance, rootID, transaction, context) {
     var markup = internalInstance.mountComponent(rootID, transaction, context);
-    if (internalInstance._currentElement.ref != null) {
+    if (internalInstance._currentElement &&
+        internalInstance._currentElement.ref != null) {
       transaction.getReactMountReady().enqueue(attachRefs, internalInstance);
     }
     return markup;
@@ -93,7 +94,9 @@ var ReactReconciler = {
 
     internalInstance.receiveComponent(nextElement, transaction, context);
 
-    if (refsChanged) {
+    if (refsChanged &&
+        internalInstance._currentElement &&
+        internalInstance._currentElement.ref != null) {
       transaction.getReactMountReady().enqueue(attachRefs, internalInstance);
     }
   },

src/renderers/shared/reconciler/ReactRef.js

@@ -34,6 +34,9 @@ function detachRef(ref, component, owner) {
 }
 
 ReactRef.attachRefs = function(instance, element) {
+  if (element === null || element === false) {
+    return;
+  }
   var ref = element.ref;
   if (ref != null) {
     attachRef(ref, instance, element._owner);
@@ -53,13 +56,21 @@ ReactRef.shouldUpdateRefs = function(prevElement, nextElement) {
   // is made. It probably belongs where the key checking and
   // instantiateReactComponent is done.
 
+  var prevEmpty = prevElement === null || prevElement === false;
+  var nextEmpty = nextElement === null || nextElement === false;
+
   return (
+    // This has a few false positives w/r/t empty components.
+    prevEmpty || nextEmpty ||
     nextElement._owner !== prevElement._owner ||
     nextElement.ref !== prevElement.ref
   );
 };
 
 ReactRef.detachRefs = function(instance, element) {
+  if (element === null || element === false) {
+    return;
+  }
   var ref = element.ref;
   if (ref != null) {
     detachRef(ref, instance, element._owner);

src/renderers/shared/reconciler/__tests__/ReactEmptyComponent-test.js

@@ -13,7 +13,6 @@
 
 var React;
 var ReactDOM;
-var ReactEmptyComponent;
 var ReactTestUtils;
 var TogglingComponent;
 
@@ -25,7 +24,6 @@ describe('ReactEmptyComponent', function() {
 
     React = require('React');
     ReactDOM = require('ReactDOM');
-    ReactEmptyComponent = require('ReactEmptyComponent');
     ReactTestUtils = require('ReactTestUtils');
 
     reactComponentExpect = require('reactComponentExpect');
@@ -64,10 +62,10 @@ describe('ReactEmptyComponent', function() {
     var instance2 = ReactTestUtils.renderIntoDocument(<Component2 />);
     reactComponentExpect(instance1)
       .expectRenderedChild()
-      .toBeComponentOfType(ReactEmptyComponent.emptyElement.type);
+      .toBeEmptyComponent();
     reactComponentExpect(instance2)
       .expectRenderedChild()
-      .toBeComponentOfType(ReactEmptyComponent.emptyElement.type);
+      .toBeEmptyComponent();
   });
 
   it('should still throw when rendering to undefined', () => {
@@ -97,10 +95,8 @@ describe('ReactEmptyComponent', function() {
         secondComponent={null}
       />;
 
-    expect(function() {
-      ReactTestUtils.renderIntoDocument(instance1);
-      ReactTestUtils.renderIntoDocument(instance2);
-    }).not.toThrow();
+    ReactTestUtils.renderIntoDocument(instance1);
+    ReactTestUtils.renderIntoDocument(instance2);
 
     expect(console.log.argsForCall.length).toBe(4);
     expect(console.log.argsForCall[0][0]).toBe(null);
@@ -269,4 +265,25 @@ describe('ReactEmptyComponent', function() {
       ReactTestUtils.renderIntoDocument(<Parent />);
     }).not.toThrow();
   });
+
+  it('preserves the dom node during updates', function() {
+    var Empty = React.createClass({
+      render: function() {
+        return null;
+      },
+    });
+
+    var container = document.createElement('div');
+
+    ReactDOM.render(<Empty />, container);
+    var noscript1 = container.firstChild;
+    expect(noscript1.tagName).toBe('NOSCRIPT');
+
+    // This update shouldn't create a DOM node
+    ReactDOM.render(<Empty />, container);
+    var noscript2 = container.firstChild;
+    expect(noscript2.tagName).toBe('NOSCRIPT');
+
+    expect(noscript1).toBe(noscript2);
+  });
 });

src/renderers/shared/reconciler/instantiateReactComponent.js

@@ -67,10 +67,8 @@ function instantiateReactComponent(node) {
   var instance;
 
   if (node === null || node === false) {
-    node = ReactEmptyComponent.emptyElement;
-  }
-
-  if (typeof node === 'object') {
+    instance = new ReactEmptyComponent(instantiateReactComponent);
+  } else if (typeof node === 'object') {
     var element = node;
     invariant(
       element && (typeof element.type === 'function' ||
@@ -86,7 +84,7 @@ function instantiateReactComponent(node) {
       instance = ReactNativeComponent.createInternalComponent(element);
     } else if (isInternalComponentType(element.type)) {
       // This is temporarily available for custom components that are not string
-      // represenations. I.e. ART. Once those are updated to use the string
+      // representations. I.e. ART. Once those are updated to use the string
       // representation, we can drop this code path.
       instance = new element.type(element);
     } else {

src/renderers/shared/reconciler/shouldUpdateReactComponent.js

@@ -24,18 +24,22 @@
  * @protected
  */
 function shouldUpdateReactComponent(prevElement, nextElement) {
-  if (prevElement != null && nextElement != null) {
-    var prevType = typeof prevElement;
-    var nextType = typeof nextElement;
-    if (prevType === 'string' || prevType === 'number') {
-      return (nextType === 'string' || nextType === 'number');
-    } else {
-      return (
-        nextType === 'object' &&
-        prevElement.type === nextElement.type &&
-        prevElement.key === nextElement.key
-      );
-    }
+  var prevEmpty = prevElement === null || prevElement === false;
+  var nextEmpty = nextElement === null || nextElement === false;
+  if (prevEmpty || nextEmpty) {
+    return prevEmpty === nextEmpty;
+  }
+
+  var prevType = typeof prevElement;
+  var nextType = typeof nextElement;
+  if (prevType === 'string' || prevType === 'number') {
+    return (nextType === 'string' || nextType === 'number');
+  } else {
+    return (
+      nextType === 'object' &&
+      prevElement.type === nextElement.type &&
+      prevElement.key === nextElement.key
+    );
   }
   return false;
 }

src/test/ReactTestUtils.js

@@ -17,7 +17,6 @@ var EventPropagators = require('EventPropagators');
 var React = require('React');
 var ReactDOM = require('ReactDOM');
 var ReactElement = require('ReactElement');
-var ReactEmptyComponent = require('ReactEmptyComponent');
 var ReactBrowserEventEmitter = require('ReactBrowserEventEmitter');
 var ReactCompositeComponent = require('ReactCompositeComponent');
 var ReactInstanceHandles = require('ReactInstanceHandles');
@@ -359,9 +358,7 @@ ReactShallowRenderer.prototype.getRenderOutput = function() {
 
 var NoopInternalComponent = function(element) {
   this._renderedOutput = element;
-  this._currentElement = element === null || element === false ?
-    ReactEmptyComponent.emptyElement :
-    element;
+  this._currentElement = element;
 };
 
 NoopInternalComponent.prototype = {
@@ -371,9 +368,7 @@ NoopInternalComponent.prototype = {
 
   receiveComponent: function(element) {
     this._renderedOutput = element;
-    this._currentElement = element === null || element === false ?
-      ReactEmptyComponent.emptyElement :
-      element;
+    this._currentElement = element;
   },
 
   unmountComponent: function() {

src/test/reactComponentExpect.js

@@ -150,6 +150,11 @@ assign(reactComponentExpectInternal.prototype, {
     return this;
   },
 
+  toBeEmptyComponent: function() {
+    var element = this._instance._currentElement;
+    return element === null || element === false;
+  },
+
   toBePresent: function() {
     expect(this.instance()).toBeTruthy();
     return this;