From 58de758a324ec75fb7ecf988dbbb645984c38857 Mon Sep 17 00:00:00 2001 From: Pete Hunt Date: Tue, 1 Oct 2013 13:31:21 -0700 Subject: [PATCH] Sort batched updates by owner depth If we reconcile components higher in the hierarchy they will likely reconcile components lower in the hierarchy. If we sort by depth then when we reach those components there will be no more pending state or props and it will no op. --- src/core/ReactComponent.js | 6 +- src/core/ReactCompositeComponent.js | 22 ++- src/core/ReactMultiChild.js | 12 +- src/core/ReactNativeComponent.js | 10 +- src/core/ReactTextComponent.js | 11 +- src/core/ReactUpdates.js | 18 +- src/core/__tests__/ReactComponent-test.js | 75 ++++++++ .../__tests__/ReactNativeComponent-test.js | 2 +- src/core/__tests__/ReactUpdates-test.js | 171 +++++++++++++++++- src/dom/__tests__/Danger-test.js | 10 +- src/environment/ReactServerRendering.js | 2 +- 11 files changed, 317 insertions(+), 22 deletions(-) diff --git a/src/core/ReactComponent.js b/src/core/ReactComponent.js index 2f048502e6..833d5ba5ac 100644 --- a/src/core/ReactComponent.js +++ b/src/core/ReactComponent.js @@ -335,10 +335,11 @@ var ReactComponent = { * * @param {string} rootID DOM ID of the root node. * @param {ReactReconcileTransaction} transaction + * @param {number} mountDepth number of components in the owner hierarchy. * @return {?string} Rendered markup to be inserted into the DOM. * @internal */ - mountComponent: function(rootID, transaction) { + mountComponent: function(rootID, transaction, mountDepth) { invariant( !this.isMounted(), 'mountComponent(%s, ...): Can only mount an unmounted component.', @@ -350,6 +351,7 @@ var ReactComponent = { } this._rootNodeID = rootID; this._lifeCycleState = ComponentLifeCycle.MOUNTED; + this._mountDepth = mountDepth; // Effectively: return ''; }, @@ -485,7 +487,7 @@ var ReactComponent = { container, transaction, shouldReuseMarkup) { - var markup = this.mountComponent(rootID, transaction); + var markup = this.mountComponent(rootID, transaction, 0); ReactComponent.mountImageIntoNode(markup, container, shouldReuseMarkup); }, diff --git a/src/core/ReactCompositeComponent.js b/src/core/ReactCompositeComponent.js index 2d4b09efd3..50405a7ddb 100644 --- a/src/core/ReactCompositeComponent.js +++ b/src/core/ReactCompositeComponent.js @@ -531,6 +531,7 @@ var ReactCompositeComponentMixin = { * * @param {string} rootID DOM ID of the root node. * @param {ReactReconcileTransaction} transaction + * @param {number} mountDepth number of components in the owner hierarchy * @return {?string} Rendered markup to be inserted into the DOM. * @final * @internal @@ -538,8 +539,13 @@ var ReactCompositeComponentMixin = { mountComponent: ReactPerf.measure( 'ReactCompositeComponent', 'mountComponent', - function(rootID, transaction) { - ReactComponent.Mixin.mountComponent.call(this, rootID, transaction); + function(rootID, transaction, mountDepth) { + ReactComponent.Mixin.mountComponent.call( + this, + rootID, + transaction, + mountDepth + ); this._compositeLifeCycleState = CompositeLifeCycle.MOUNTING; this._defaultProps = this.getDefaultProps ? this.getDefaultProps() : null; @@ -567,7 +573,11 @@ var ReactCompositeComponentMixin = { // Done with mounting, `setState` will now trigger UI changes. this._compositeLifeCycleState = null; - var markup = this._renderedComponent.mountComponent(rootID, transaction); + var markup = this._renderedComponent.mountComponent( + rootID, + transaction, + mountDepth + 1 + ); if (this.componentDidMount) { transaction.getReactOnDOMReady().enqueue(this, this.componentDidMount); } @@ -788,7 +798,11 @@ var ReactCompositeComponentMixin = { var thisID = this._rootNodeID; var currentComponentID = currentComponent._rootNodeID; currentComponent.unmountComponent(); - var nextMarkup = nextComponent.mountComponent(thisID, transaction); + var nextMarkup = nextComponent.mountComponent( + thisID, + transaction, + this._mountDepth + 1 + ); ReactComponent.DOMIDOperations.dangerouslyReplaceNodeWithMarkupByID( currentComponentID, nextMarkup diff --git a/src/core/ReactMultiChild.js b/src/core/ReactMultiChild.js index 79c493423d..affb0d67c8 100644 --- a/src/core/ReactMultiChild.js +++ b/src/core/ReactMultiChild.js @@ -202,7 +202,11 @@ var ReactMultiChild = { if (children.hasOwnProperty(name) && child) { // Inlined for performance, see `ReactInstanceHandles.createReactID`. var rootID = this._rootNodeID + '.' + name; - var mountImage = child.mountComponent(rootID, transaction); + var mountImage = child.mountComponent( + rootID, + transaction, + this._mountDepth + 1 + ); child._mountImage = mountImage; child._mountIndex = index; mountImages.push(mountImage); @@ -395,7 +399,11 @@ var ReactMultiChild = { _mountChildByNameAtIndex: function(child, name, index, transaction) { // Inlined for performance, see `ReactInstanceHandles.createReactID`. var rootID = this._rootNodeID + '.' + name; - var mountImage = child.mountComponent(rootID, transaction); + var mountImage = child.mountComponent( + rootID, + transaction, + this._mountDepth + 1 + ); child._mountImage = mountImage; child._mountIndex = index; this.createChild(child); diff --git a/src/core/ReactNativeComponent.js b/src/core/ReactNativeComponent.js index 2ccc69b44f..4fc4d67da8 100644 --- a/src/core/ReactNativeComponent.js +++ b/src/core/ReactNativeComponent.js @@ -84,13 +84,19 @@ ReactNativeComponent.Mixin = { * @internal * @param {string} rootID The root DOM ID for this node. * @param {ReactReconcileTransaction} transaction + * @param {number} mountDepth number of components in the owner hierarchy * @return {string} The computed markup. */ mountComponent: ReactPerf.measure( 'ReactNativeComponent', 'mountComponent', - function(rootID, transaction) { - ReactComponent.Mixin.mountComponent.call(this, rootID, transaction); + function(rootID, transaction, mountDepth) { + ReactComponent.Mixin.mountComponent.call( + this, + rootID, + transaction, + mountDepth + ); assertValidProps(this.props); return ( this._createOpenTagMarkup() + diff --git a/src/core/ReactTextComponent.js b/src/core/ReactTextComponent.js index 24c07bd704..235f0dad41 100644 --- a/src/core/ReactTextComponent.js +++ b/src/core/ReactTextComponent.js @@ -52,11 +52,18 @@ mixInto(ReactTextComponent, { * any features besides containing text content. * * @param {string} rootID DOM ID of the root node. + * @param {ReactReconcileTransaction} transaction + * @param {number} mountDepth number of components in the owner hierarchy * @return {string} Markup for this text node. * @internal */ - mountComponent: function(rootID) { - ReactComponent.Mixin.mountComponent.call(this, rootID); + mountComponent: function(rootID, transaction, mountDepth) { + ReactComponent.Mixin.mountComponent.call( + this, + rootID, + transaction, + mountDepth + ); return ( '' + escapeTextForBrowser(this.props.text) + diff --git a/src/core/ReactUpdates.js b/src/core/ReactUpdates.js index bcb1f68c78..d866eec089 100644 --- a/src/core/ReactUpdates.js +++ b/src/core/ReactUpdates.js @@ -33,8 +33,24 @@ function batchedUpdates(callback, param) { batchingStrategy.batchedUpdates(callback, param); } +/** + * Array comparator for ReactComponents by owner depth + * + * @param {ReactComponent} c1 first component you're comparing + * @param {ReactComponent} c2 second component you're comparing + * @return {number} Return value usable by Array.prototype.sort(). + */ +function mountDepthComparator(c1, c2) { + return c1._mountDepth - c2._mountDepth; +} + function runBatchedUpdates() { - // TODO: Sort components by depth such that parent components update first + // Since reconciling a component higher in the owner hierarchy usually (not + // always -- see shouldComponentUpdate()) will reconcile children, reconcile + // them before their children by sorting the array. + + dirtyComponents.sort(mountDepthComparator); + for (var i = 0; i < dirtyComponents.length; 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 diff --git a/src/core/__tests__/ReactComponent-test.js b/src/core/__tests__/ReactComponent-test.js index 9c47a1e0f1..95e0882bd5 100644 --- a/src/core/__tests__/ReactComponent-test.js +++ b/src/core/__tests__/ReactComponent-test.js @@ -133,4 +133,79 @@ describe('ReactComponent', function() { expect(instance.isMounted()).toBeTruthy(); }); + it('should know its simple mount depth', function() { + var Owner = React.createClass({ + render: function() { + return ; + } + }); + + var Child = React.createClass({ + render: function() { + return
; + } + }); + + var instance = ; + ReactTestUtils.renderIntoDocument(instance); + expect(instance._mountDepth).toBe(0); + expect(instance.refs.child._mountDepth).toBe(1); + }); + + it('should know its (complicated) mount depth', function() { + var Box = React.createClass({ + render: function() { + return
{this.props.children}
; + } + }); + + var Child = React.createClass({ + render: function() { + return child; + } + }); + + var Switcher = React.createClass({ + getInitialState: function() { + return {tabKey: 'hello'}; + }, + + render: function() { + var child = this.props.children; + + return ( + +
+ {child} +
+ + ); + } + }); + + var App = React.createClass({ + render: function() { + return ( + + + + ); + } + }); + + var root = ; + ReactTestUtils.renderIntoDocument(root); + + expect(root._mountDepth).toBe(0); + expect(root.refs.switcher._mountDepth).toBe(1); + expect(root.refs.switcher.refs.box._mountDepth).toBe(2); + expect(root.refs.switcher.refs.switcherDiv._mountDepth).toBe(4); + expect(root.refs.child._mountDepth).toBe(5); + expect(root.refs.switcher.refs.box.refs.boxDiv._mountDepth).toBe(3); + expect(root.refs.child.refs.span._mountDepth).toBe(6); + }); }); diff --git a/src/core/__tests__/ReactNativeComponent-test.js b/src/core/__tests__/ReactNativeComponent-test.js index 29a9498a86..f7c7dae3d1 100644 --- a/src/core/__tests__/ReactNativeComponent-test.js +++ b/src/core/__tests__/ReactNativeComponent-test.js @@ -290,7 +290,7 @@ describe('ReactNativeComponent', function() { mountComponent = function(props) { var transaction = new ReactReconcileTransaction(); var stubComponent = new StubNativeComponent(props); - return stubComponent.mountComponent('test', transaction); + return stubComponent.mountComponent('test', transaction, 0); }; }); diff --git a/src/core/__tests__/ReactUpdates-test.js b/src/core/__tests__/ReactUpdates-test.js index d73bad3739..ee0267b5c8 100644 --- a/src/core/__tests__/ReactUpdates-test.js +++ b/src/core/__tests__/ReactUpdates-test.js @@ -210,10 +210,8 @@ describe('ReactUpdates', function() { expect(child.state.y).toBe(2); expect(parentUpdateCount).toBe(1); - // When we update the child first, we currently incur two updates because - // we aren't smart about what order to process the components in. - // TODO: Reduce the update count here to 1 - expect(childUpdateCount).toBe(2); + // Batching reduces the number of updates here to 1. + expect(childUpdateCount).toBe(1); }); it('should support chained state updates', function() { @@ -293,4 +291,169 @@ describe('ReactUpdates', function() { expect(instance.state.x).toBe(1); expect(updateCount).toBe(1); }); + + it('should update children even if parent blocks updates', function() { + var parentRenderCount = 0; + var childRenderCount = 0; + + var Parent = React.createClass({ + shouldComponentUpdate: function() { + return false; + }, + + render: function() { + parentRenderCount++; + return ; + } + }); + + var Child = React.createClass({ + render: function() { + childRenderCount++; + return
; + } + }); + + expect(parentRenderCount).toBe(0); + expect(childRenderCount).toBe(0); + + var instance = ; + ReactTestUtils.renderIntoDocument(instance); + + expect(parentRenderCount).toBe(1); + expect(childRenderCount).toBe(1); + + ReactUpdates.batchedUpdates(function() { + instance.setState({x: 1}); + }); + + expect(parentRenderCount).toBe(1); + expect(childRenderCount).toBe(1); + + ReactUpdates.batchedUpdates(function() { + instance.refs.child.setState({x: 1}); + }); + + expect(parentRenderCount).toBe(1); + expect(childRenderCount).toBe(2); + }); + + it('should flow updates correctly', function() { + var willUpdates = []; + var didUpdates = []; + + var UpdateLoggingMixin = { + componentWillUpdate: function() { + willUpdates.push(this.constructor.displayName); + }, + componentDidUpdate: function() { + didUpdates.push(this.constructor.displayName); + } + }; + + var Box = React.createClass({ + mixins: [UpdateLoggingMixin], + + render: function() { + return
{this.props.children}
; + } + }); + + var Child = React.createClass({ + mixins: [UpdateLoggingMixin], + + render: function() { + return child; + } + }); + + var Switcher = React.createClass({ + mixins: [UpdateLoggingMixin], + + getInitialState: function() { + return {tabKey: 'hello'}; + }, + + render: function() { + var child = this.props.children; + + return ( + +
+ {child} +
+ + ); + } + }); + + var App = React.createClass({ + mixins: [UpdateLoggingMixin], + + render: function() { + return ( + + + + ); + } + }); + + var root = ; + ReactTestUtils.renderIntoDocument(root); + + function expectUpdates(sequence) { + // didUpdate() occurs in reverse order + didUpdates.reverse(); + expect(willUpdates).toEqual(didUpdates); + expect(willUpdates).toEqual(sequence); + willUpdates.length = 0; + didUpdates.length = 0; + } + + function triggerUpdate(c) { + c.setState({x: 1}); + } + + function testUpdates(components, expectation) { + var i; + + ReactUpdates.batchedUpdates(function() { + for (i = 0; i < components.length; i++) { + triggerUpdate(components[i]); + } + }); + + expectUpdates(expectation); + + // Try them in reverse order + + ReactUpdates.batchedUpdates(function() { + for (i = components.length - 1; i >= 0; i--) { + triggerUpdate(components[i]); + } + }); + + expectUpdates(expectation); + } + + testUpdates( + [root.refs.switcher.refs.box, root.refs.switcher], + ['Switcher', 'Box', 'Child'] + ); + + testUpdates( + [root.refs.child, root.refs.switcher.refs.box], + ['Box', 'Child'] + ); + + testUpdates( + [root.refs.child, root.refs.switcher], + ['Switcher', 'Box', 'Child'] + ); + }); }); diff --git a/src/dom/__tests__/Danger-test.js b/src/dom/__tests__/Danger-test.js index d3f94a4244..f82a97241e 100644 --- a/src/dom/__tests__/Danger-test.js +++ b/src/dom/__tests__/Danger-test.js @@ -36,14 +36,18 @@ describe('Danger', function() { }); it('should render markup', function() { - var markup = (
).mountComponent('.rX', transaction); + var markup = (
).mountComponent('.rX', transaction, 0); var output = Danger.dangerouslyRenderMarkup([markup])[0]; expect(output.nodeName).toBe('DIV'); }); it('should render markup with props', function() { - var markup = (
).mountComponent('.rX', transaction); + var markup = (
).mountComponent( + '.rX', + transaction, + 0 + ); var output = Danger.dangerouslyRenderMarkup([markup])[0]; expect(output.nodeName).toBe('DIV'); @@ -51,7 +55,7 @@ describe('Danger', function() { }); it('should render wrapped markup', function() { - var markup = ().mountComponent('.rX', transaction); + var markup = ().mountComponent('.rX', transaction, 0); var output = Danger.dangerouslyRenderMarkup([markup])[0]; expect(output.nodeName).toBe('TH'); diff --git a/src/environment/ReactServerRendering.js b/src/environment/ReactServerRendering.js index 748f9f35cc..626cf117b0 100644 --- a/src/environment/ReactServerRendering.js +++ b/src/environment/ReactServerRendering.js @@ -34,7 +34,7 @@ function renderComponentToString(component, callback) { transaction.reinitializeTransaction(); try { transaction.perform(function() { - var markup = component.mountComponent(id, transaction); + var markup = component.mountComponent(id, transaction, 0); markup = ReactMarkupChecksum.addChecksumToMarkup(markup); callback(markup); }, null);