Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use comment nodes for empty components #5451

Merged
merged 1 commit into from
Nov 20, 2015
Merged

Use comment nodes for empty components #5451

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/renderers/dom/client/ReactDOMComponentTree.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
2 changes: 1 addition & 1 deletion src/renderers/dom/shared/Danger.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ var Danger = {
);
invariant(markup, 'dangerouslyReplaceNodeWithMarkup(...): Missing markup.');
invariant(
oldChild.tagName.toLowerCase() !== 'html',
oldChild.nodeName !== 'HTML',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://developer.mozilla.org/en-US/docs/Web/API/Node/nodeName indicates that nodeName should be 'html' for XHTML docs but that doesn't appear to be true in practice (for several years, the linked article there is from 2009) so this is probably safe.

'dangerouslyReplaceNodeWithMarkup(...): Cannot replace markup of the ' +
'<html> node. This is because browser quirks make this unreliable ' +
'and/or slow. If you want to render to the root you must use ' +
Expand Down
68 changes: 68 additions & 0 deletions src/renderers/dom/shared/ReactDOMEmptyComponent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 '<!--' + nodeValue + '-->';
}
},
receiveComponent: function() {
},
getNativeNode: function() {
return ReactDOMComponentTree.getNodeFromInstance(this);
},
unmountComponent: function() {
ReactDOMComponentTree.uncacheNode(this);
},
});

module.exports = ReactDOMEmptyComponent;
2 changes: 1 addition & 1 deletion src/renderers/dom/shared/ReactDOMTextComponent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
7 changes: 6 additions & 1 deletion src/renderers/dom/shared/ReactDefaultInjection.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down Expand Up @@ -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
Expand Down
45 changes: 7 additions & 38 deletions src/renderers/shared/reconciler/ReactEmptyComponent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
50 changes: 50 additions & 0 deletions src/renderers/shared/reconciler/ReactSimpleEmptyComponent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't use this anywhere… is the thinking that this would be used in RN?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. (I even mentioned that in my PR description. ;))

*/

'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;
28 changes: 26 additions & 2 deletions src/renderers/shared/reconciler/__tests__/ReactEmptyComponent-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
<TogglingComponent
firstComponent={null}
secondComponent={'div'}
/>;

ReactTestUtils.renderIntoDocument(
<div>
{instance1}
{instance1}
{instance1}
</div>
);

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 =
Expand Down Expand Up @@ -275,12 +299,12 @@ describe('ReactEmptyComponent', function() {

ReactDOM.render(<Empty />, 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(<Empty />, container);
var noscript2 = container.firstChild;
expect(noscript2.tagName).toBe('NOSCRIPT');
expect(noscript2.nodeName).toBe('#comment');

expect(noscript1).toBe(noscript2);
});
Expand Down
2 changes: 1 addition & 1 deletion src/renderers/shared/reconciler/instantiateReactComponent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down