From 358140679cbda686f8c3a7d04887342188572cb0 Mon Sep 17 00:00:00 2001 From: Ben Alpert Date: Wed, 11 Nov 2015 10:04:14 -0800 Subject: [PATCH] Use comment nodes for empty components This makes more sense and avoids DOM nesting problems. ![image](https://cloud.githubusercontent.com/assets/6820/11098713/952348ca-885b-11e5-9757-e4a76467b0b8.png) (ReactSimpleEmptyComponent isn't used here but React Native can use it as it currently does.) --- .../dom/client/ReactDOMComponentTree.js | 4 +- src/renderers/dom/shared/Danger.js | 2 +- .../dom/shared/ReactDOMEmptyComponent.js | 68 +++++++++++++++++++ .../dom/shared/ReactDOMTextComponent.js | 2 +- .../dom/shared/ReactDefaultInjection.js | 7 +- .../shared/reconciler/ReactEmptyComponent.js | 45 ++---------- .../reconciler/ReactSimpleEmptyComponent.js | 50 ++++++++++++++ .../__tests__/ReactEmptyComponent-test.js | 28 +++++++- .../reconciler/instantiateReactComponent.js | 2 +- 9 files changed, 163 insertions(+), 45 deletions(-) create mode 100644 src/renderers/dom/shared/ReactDOMEmptyComponent.js create mode 100644 src/renderers/shared/reconciler/ReactSimpleEmptyComponent.js diff --git a/src/renderers/dom/client/ReactDOMComponentTree.js b/src/renderers/dom/client/ReactDOMComponentTree.js index 53d29d2dca608..6bdc669fab48f 100644 --- a/src/renderers/dom/client/ReactDOMComponentTree.js +++ b/src/renderers/dom/client/ReactDOMComponentTree.js @@ -85,7 +85,9 @@ function precacheChildNodes(inst, node) { // We assume the child nodes are in the same order as the child instances. for (; childNode !== null; childNode = childNode.nextSibling) { if (childNode.nodeType === 1 && - childNode.getAttribute(ATTR_NAME) === String(childID)) { + childNode.getAttribute(ATTR_NAME) === String(childID) || + childNode.nodeType === 8 && + childNode.nodeValue === ' react-empty: ' + childID + ' ') { precacheNode(childInst, childNode); continue outer; } diff --git a/src/renderers/dom/shared/Danger.js b/src/renderers/dom/shared/Danger.js index 04dbe88a3571a..fb544acd93c60 100644 --- a/src/renderers/dom/shared/Danger.js +++ b/src/renderers/dom/shared/Danger.js @@ -166,7 +166,7 @@ var Danger = { ); invariant(markup, 'dangerouslyReplaceNodeWithMarkup(...): Missing markup.'); invariant( - oldChild.tagName.toLowerCase() !== 'html', + oldChild.nodeName !== 'HTML', 'dangerouslyReplaceNodeWithMarkup(...): Cannot replace markup of the ' + ' node. This is because browser quirks make this unreliable ' + 'and/or slow. If you want to render to the root you must use ' + diff --git a/src/renderers/dom/shared/ReactDOMEmptyComponent.js b/src/renderers/dom/shared/ReactDOMEmptyComponent.js new file mode 100644 index 0000000000000..d7c9e825d997f --- /dev/null +++ b/src/renderers/dom/shared/ReactDOMEmptyComponent.js @@ -0,0 +1,68 @@ +/** + * 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 ReactDOMEmptyComponent + */ + +'use strict'; + +var DOMLazyTree = require('DOMLazyTree'); +var ReactDOMComponentTree = require('ReactDOMComponentTree'); + +var assign = require('Object.assign'); + +var ReactDOMEmptyComponent = function(instantiate) { + // ReactCompositeComponent uses this: + this._currentElement = null; + // ReactDOMComponentTree uses these: + this._nativeNode = null; + this._nativeParent = null; + this._nativeContainerInfo = null; + this._domID = null; +}; +assign(ReactDOMEmptyComponent.prototype, { + construct: function(element) { + }, + mountComponent: function( + transaction, + nativeParent, + nativeContainerInfo, + context + ) { + var domID = nativeContainerInfo._idCounter++; + this._domID = domID; + this._nativeParent = nativeParent; + this._nativeContainerInfo = nativeContainerInfo; + + var nodeValue = ' react-empty: ' + this._domID + ' '; + if (transaction.useCreateElement) { + var ownerDocument = nativeContainerInfo._ownerDocument; + var node = ownerDocument.createComment(nodeValue); + ReactDOMComponentTree.precacheNode(this, node); + return DOMLazyTree(node); + } else { + if (transaction.renderToStaticMarkup) { + // Normally we'd insert a comment node, but since this is a situation + // where React won't take over (static pages), we can simply return + // nothing. + return ''; + } + return ''; + } + }, + receiveComponent: function() { + }, + getNativeNode: function() { + return ReactDOMComponentTree.getNodeFromInstance(this); + }, + unmountComponent: function() { + ReactDOMComponentTree.uncacheNode(this); + }, +}); + +module.exports = ReactDOMEmptyComponent; diff --git a/src/renderers/dom/shared/ReactDOMTextComponent.js b/src/renderers/dom/shared/ReactDOMTextComponent.js index 262748fc6dfda..539b3ebafca41 100644 --- a/src/renderers/dom/shared/ReactDOMTextComponent.js +++ b/src/renderers/dom/shared/ReactDOMTextComponent.js @@ -53,8 +53,8 @@ assign(ReactDOMTextComponent.prototype, { // TODO: This is really a ReactText (ReactNode), not a ReactElement this._currentElement = text; this._stringText = '' + text; + // ReactDOMComponentTree uses these: this._nativeNode = null; - // ReactDOMComponentTree uses this: this._nativeParent = null; // Properties diff --git a/src/renderers/dom/shared/ReactDefaultInjection.js b/src/renderers/dom/shared/ReactDefaultInjection.js index b6f8edd745fe7..2368d7e112391 100644 --- a/src/renderers/dom/shared/ReactDefaultInjection.js +++ b/src/renderers/dom/shared/ReactDefaultInjection.js @@ -22,6 +22,7 @@ var ReactComponentBrowserEnvironment = require('ReactComponentBrowserEnvironment'); var ReactDOMComponent = require('ReactDOMComponent'); var ReactDOMComponentTree = require('ReactDOMComponentTree'); +var ReactDOMEmptyComponent = require('ReactDOMEmptyComponent'); var ReactDOMTreeTraversal = require('ReactDOMTreeTraversal'); var ReactDOMTextComponent = require('ReactDOMTextComponent'); var ReactDefaultBatchingStrategy = require('ReactDefaultBatchingStrategy'); @@ -79,7 +80,11 @@ function inject() { ReactInjection.DOMProperty.injectDOMPropertyConfig(HTMLDOMPropertyConfig); ReactInjection.DOMProperty.injectDOMPropertyConfig(SVGDOMPropertyConfig); - ReactInjection.EmptyComponent.injectEmptyComponent('noscript'); + ReactInjection.EmptyComponent.injectEmptyComponentFactory( + function(instantiate) { + return new ReactDOMEmptyComponent(instantiate); + } + ); ReactInjection.Updates.injectReconcileTransaction( ReactReconcileTransaction diff --git a/src/renderers/shared/reconciler/ReactEmptyComponent.js b/src/renderers/shared/reconciler/ReactEmptyComponent.js index 3095638b4ec70..6a12c376ded4f 100644 --- a/src/renderers/shared/reconciler/ReactEmptyComponent.js +++ b/src/renderers/shared/reconciler/ReactEmptyComponent.js @@ -11,50 +11,19 @@ 'use strict'; -var ReactElement = require('ReactElement'); -var ReactReconciler = require('ReactReconciler'); - -var assign = require('Object.assign'); - -var placeholderElement; +var emptyComponentFactory; var ReactEmptyComponentInjection = { - injectEmptyComponent: function(component) { - placeholderElement = ReactElement.createElement(component); + injectEmptyComponentFactory: function(factory) { + emptyComponentFactory = factory; }, }; -var ReactEmptyComponent = function(instantiate) { - this._currentElement = null; - this._renderedComponent = instantiate(placeholderElement); -}; -assign(ReactEmptyComponent.prototype, { - construct: function(element) { - }, - mountComponent: function( - transaction, - nativeParent, - nativeContainerInfo, - context - ) { - return ReactReconciler.mountComponent( - this._renderedComponent, - transaction, - nativeParent, - nativeContainerInfo, - context - ); +var ReactEmptyComponent = { + create: function(instantiate) { + return emptyComponentFactory(instantiate); }, - receiveComponent: function() { - }, - getNativeNode: function() { - return ReactReconciler.getNativeNode(this._renderedComponent); - }, - unmountComponent: function() { - ReactReconciler.unmountComponent(this._renderedComponent); - this._renderedComponent = null; - }, -}); +}; ReactEmptyComponent.injection = ReactEmptyComponentInjection; diff --git a/src/renderers/shared/reconciler/ReactSimpleEmptyComponent.js b/src/renderers/shared/reconciler/ReactSimpleEmptyComponent.js new file mode 100644 index 0000000000000..5419fc12f68c3 --- /dev/null +++ b/src/renderers/shared/reconciler/ReactSimpleEmptyComponent.js @@ -0,0 +1,50 @@ +/** + * 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 ReactSimpleEmptyComponent + */ + +'use strict'; + +var ReactReconciler = require('ReactReconciler'); + +var assign = require('Object.assign'); + +var ReactSimpleEmptyComponent = function(placeholderElement, instantiate) { + this._currentElement = null; + this._renderedComponent = instantiate(placeholderElement); +}; +assign(ReactSimpleEmptyComponent.prototype, { + construct: function(element) { + }, + mountComponent: function( + transaction, + nativeParent, + nativeContainerInfo, + context + ) { + return ReactReconciler.mountComponent( + this._renderedComponent, + transaction, + nativeParent, + nativeContainerInfo, + context + ); + }, + receiveComponent: function() { + }, + getNativeNode: function() { + return ReactReconciler.getNativeNode(this._renderedComponent); + }, + unmountComponent: function() { + ReactReconciler.unmountComponent(this._renderedComponent); + this._renderedComponent = null; + }, +}); + +module.exports = ReactSimpleEmptyComponent; diff --git a/src/renderers/shared/reconciler/__tests__/ReactEmptyComponent-test.js b/src/renderers/shared/reconciler/__tests__/ReactEmptyComponent-test.js index cf88eaeba3ba1..24fea6a90beb4 100644 --- a/src/renderers/shared/reconciler/__tests__/ReactEmptyComponent-test.js +++ b/src/renderers/shared/reconciler/__tests__/ReactEmptyComponent-test.js @@ -106,6 +106,30 @@ describe('ReactEmptyComponent', function() { expect(log.argsForCall[3][0]).toBe(null); }); + it('should be able to switch in a list of children', () => { + var instance1 = + ; + + ReactTestUtils.renderIntoDocument( +
+ {instance1} + {instance1} + {instance1} +
+ ); + + expect(log.argsForCall.length).toBe(6); + expect(log.argsForCall[0][0]).toBe(null); + expect(log.argsForCall[1][0]).toBe(null); + expect(log.argsForCall[2][0]).toBe(null); + expect(log.argsForCall[3][0].tagName).toBe('DIV'); + expect(log.argsForCall[4][0].tagName).toBe('DIV'); + expect(log.argsForCall[5][0].tagName).toBe('DIV'); + }); + it('should distinguish between a script placeholder and an actual script tag', () => { var instance1 = @@ -275,12 +299,12 @@ describe('ReactEmptyComponent', function() { ReactDOM.render(, container); var noscript1 = container.firstChild; - expect(noscript1.tagName).toBe('NOSCRIPT'); + expect(noscript1.nodeName).toBe('#comment'); // This update shouldn't create a DOM node ReactDOM.render(, container); var noscript2 = container.firstChild; - expect(noscript2.tagName).toBe('NOSCRIPT'); + expect(noscript2.nodeName).toBe('#comment'); expect(noscript1).toBe(noscript2); }); diff --git a/src/renderers/shared/reconciler/instantiateReactComponent.js b/src/renderers/shared/reconciler/instantiateReactComponent.js index 588014dc938ed..92555e0018615 100644 --- a/src/renderers/shared/reconciler/instantiateReactComponent.js +++ b/src/renderers/shared/reconciler/instantiateReactComponent.js @@ -67,7 +67,7 @@ function instantiateReactComponent(node) { var instance; if (node === null || node === false) { - instance = new ReactEmptyComponent(instantiateReactComponent); + instance = ReactEmptyComponent.create(instantiateReactComponent); } else if (typeof node === 'object') { var element = node; invariant(