From de1dacdb2887349d09b839bb79d008ae0bf057b0 Mon Sep 17 00:00:00 2001 From: Ben Alpert Date: Wed, 30 Jul 2014 14:02:58 -0700 Subject: [PATCH] Refactor composite component update flow Fixes #1392. Previously, ReactComponent.updateComponent set the new props and owner and updated the component's refs. Because it did the actual props assignment, it had to be called by ReactCompositeComponent between componentWillMount and componentDidMount, meaning that it was skipped if shouldComponentUpdate returned false. Now, updateComponent takes the old and new descriptors and only updates refs, allowing a subclass to call updateComponent at a convenient time (specifically, it can be before `this._descriptor` is updated if the subclass also overrides performUpdateIfNecessary). The new update cycle for composites is: - receiveComponent is called which stores `this._pendingDescriptor` and calls performUpdateIfNecessary directly or a state update is enqueued, in which case ReactUpdates will call performUpdateIfNecessary - performUpdateIfNecessary ensures that an update is still pending (which might no longer be the case if an update is queued for a subcomponent that was updated through a parent's reconciliation) and calls updateComponent with the old and new descriptors - updateComponent calls super to update refs, then calls componentWillReceiveProps (if applicable) and shouldComponentUpdate -- if shouldComponentUpdate returns false, `this._descriptor`, `this.props`, and `this.state` are updated and the update is skipped; else, _performComponentUpdate is called - _performComponentUpdate calls componentWillUpdate and _updateRenderedComponent, and enqueues the componentDidUpdate call (in that order) - _updateRenderedComponent calls render and updates the rendered component appropriately For DOM components (essentially unchanged): - receiveComponent is called which does a short-circuit check for descriptor equality and delegates to super (which stores `this._pendingDescriptor` and calls performUpdateIfNecessary) - performUpdateIfNecessary (not overridden) sets new values for `this.props`, etc. and calls updateComponent - updateComponent does the DOM property and children updates (some synchronously, some queued for the transaction close) For text and ART components (unchanged): - receiveComponent updates the DOM or ART directly based on the new props - updateComponent is never called Notable changes: - When shouldComponentUpdate returns false, refs are still updated - ReactDefaultPerf now includes lifecycle methods in component timings - updateComponent's signature changed (technically this is part of the public API, though we've never documented it) Test Plan: jest --- src/browser/ReactTextComponent.js | 11 + src/browser/ui/ReactDOMComponent.js | 6 +- src/core/ReactComponent.js | 7 +- src/core/ReactCompositeComponent.js | 235 ++++++++++-------- .../__tests__/ReactCompositeComponent-test.js | 36 +++ 5 files changed, 185 insertions(+), 110 deletions(-) diff --git a/src/browser/ReactTextComponent.js b/src/browser/ReactTextComponent.js index f87930f3f7..8750377391 100644 --- a/src/browser/ReactTextComponent.js +++ b/src/browser/ReactTextComponent.js @@ -18,6 +18,7 @@ var ReactElement = require('ReactElement'); var assign = require('Object.assign'); var escapeTextForBrowser = require('escapeTextForBrowser'); +var invariant = require('invariant'); /** * Text nodes violate a couple assumptions that React makes about components: @@ -84,12 +85,22 @@ assign(ReactTextComponent.prototype, ReactComponent.Mixin, { receiveComponent: function(nextComponent, transaction) { var nextProps = nextComponent.props; if (nextProps !== this.props) { + // TODO: Save this as pending props and use performUpdateIfNecessary + // and/or updateComponent to do the actual update for consistency with + // other component types? this.props = nextProps; ReactComponent.BackendIDOperations.updateTextContentByID( this._rootNodeID, nextProps ); } + }, + + updateComponent: function() { + invariant( + false, + 'ReactTextComponent: updateComponent() should never be called' + ); } }); diff --git a/src/browser/ui/ReactDOMComponent.js b/src/browser/ui/ReactDOMComponent.js index 72a4aadf8b..71f80ecfbb 100644 --- a/src/browser/ui/ReactDOMComponent.js +++ b/src/browser/ui/ReactDOMComponent.js @@ -303,18 +303,20 @@ ReactDOMComponent.Mixin = { * * @param {ReactReconcileTransaction} transaction * @param {ReactElement} prevElement + * @param {ReactElement} nextElement * @internal * @overridable */ updateComponent: ReactPerf.measure( 'ReactDOMComponent', 'updateComponent', - function(transaction, prevElement) { + function(transaction, prevElement, nextElement) { assertValidProps(this._currentElement.props); ReactComponent.Mixin.updateComponent.call( this, transaction, - prevElement + prevElement, + nextElement ); this._updateDOMProperties(prevElement.props, transaction); this._updateDOMChildren(prevElement.props, transaction); diff --git a/src/core/ReactComponent.js b/src/core/ReactComponent.js index bc9e0c01d5..9d4e4b7e58 100644 --- a/src/core/ReactComponent.js +++ b/src/core/ReactComponent.js @@ -323,7 +323,7 @@ var ReactComponent = { this.props = nextElement.props; this._owner = nextElement._owner; this._pendingElement = null; - this.updateComponent(transaction, prevElement); + this.updateComponent(transaction, prevElement, nextElement); }, /** @@ -331,11 +331,10 @@ var ReactComponent = { * * @param {ReactReconcileTransaction} transaction * @param {object} prevElement + * @param {object} nextElement * @internal */ - updateComponent: function(transaction, prevElement) { - var nextElement = this._currentElement; - + updateComponent: function(transaction, prevElement, nextElement) { // If either the owner or a `ref` has changed, make sure the newest owner // has stored a reference to `this`, and the previous owner (if different) // has forgotten the reference to `this`. We use the element instead diff --git a/src/core/ReactCompositeComponent.js b/src/core/ReactCompositeComponent.js index 1b063b9064..24dfc9f5af 100644 --- a/src/core/ReactCompositeComponent.js +++ b/src/core/ReactCompositeComponent.js @@ -1006,6 +1006,26 @@ var ReactCompositeComponentMixin = { } }, + receiveComponent: function(nextElement, transaction) { + if (nextElement === this._currentElement && + nextElement._owner != null) { + // Since elements are immutable after the owner is rendered, + // we can do a cheap identity compare here to determine if this is a + // superfluous reconcile. It's possible for state to be mutable but such + // change should trigger an update of the owner which would recreate + // the element. We explicitly check for the existence of an owner since + // it's possible for an element created outside a composite to be + // deeply mutated and reused. + return; + } + + ReactComponent.Mixin.receiveComponent.call( + this, + nextElement, + transaction + ); + }, + /** * If any of `_pendingElement`, `_pendingState`, or `_pendingForceUpdate` * is set, update the component. @@ -1028,64 +1048,107 @@ var ReactCompositeComponentMixin = { return; } - var nextContext = this.context; - var nextProps = this.props; - var nextElement = this._currentElement; + var prevElement = this._currentElement; + var nextElement = prevElement; if (this._pendingElement != null) { nextElement = this._pendingElement; - nextContext = this._processContext(nextElement._context); - nextProps = this._processProps(nextElement.props); this._pendingElement = null; - - this._compositeLifeCycleState = CompositeLifeCycle.RECEIVING_PROPS; - if (this.componentWillReceiveProps) { - this.componentWillReceiveProps(nextProps, nextContext); - } } - this._compositeLifeCycleState = null; + this.updateComponent( + transaction, + prevElement, + nextElement + ); + }, - var nextState = this._pendingState || this.state; - this._pendingState = null; + /** + * Perform an update to a mounted component. The componentWillReceiveProps and + * shouldComponentUpdate methods are called, then (assuming the update isn't + * skipped) the remaining update lifecycle methods are called and the DOM + * representation is updated. + * + * By default, this implements React's rendering and reconciliation algorithm. + * Sophisticated clients may wish to override this. + * + * @param {ReactReconcileTransaction} transaction + * @param {ReactElement} prevParentElement + * @param {ReactElement} nextParentElement + * @internal + * @overridable + */ + updateComponent: ReactPerf.measure( + 'ReactCompositeComponent', + 'updateComponent', + function(transaction, prevParentElement, nextParentElement) { + // Update refs regardless of what shouldComponentUpdate returns + ReactComponent.Mixin.updateComponent.call( + this, + transaction, + prevParentElement, + nextParentElement + ); - var shouldUpdate = - this._pendingForceUpdate || - !this.shouldComponentUpdate || - this.shouldComponentUpdate(nextProps, nextState, nextContext); + var prevContext = this.context; + var prevProps = this.props; + var nextContext = prevContext; + var nextProps = prevProps; + // Distinguish between a props update versus a simple state update + if (prevParentElement !== nextParentElement) { + nextContext = this._processContext(nextParentElement._context); + nextProps = this._processProps(nextParentElement.props); - if (__DEV__) { - if (typeof shouldUpdate === "undefined") { - console.warn( - (this.constructor.displayName || 'ReactCompositeComponent') + - '.shouldComponentUpdate(): Returned undefined instead of a ' + - 'boolean value. Make sure to return true or false.' - ); + this._compositeLifeCycleState = CompositeLifeCycle.RECEIVING_PROPS; + if (this.componentWillReceiveProps) { + this.componentWillReceiveProps(nextProps, nextContext); + } + } + + this._compositeLifeCycleState = null; + + var nextState = this._pendingState || this.state; + this._pendingState = null; + + var shouldUpdate = + this._pendingForceUpdate || + !this.shouldComponentUpdate || + this.shouldComponentUpdate(nextProps, nextState, nextContext); + + if (__DEV__) { + if (typeof shouldUpdate === "undefined") { + console.warn( + (this.constructor.displayName || 'ReactCompositeComponent') + + '.shouldComponentUpdate(): Returned undefined instead of a ' + + 'boolean value. Make sure to return true or false.' + ); + } + } + + if (!shouldUpdate) { + // If it's determined that a component should not update, we still want + // to set props and state but we shortcut the rest of the update. + this._currentElement = nextParentElement; + this.props = nextProps; + this.state = nextState; + this.context = nextContext; + + // Owner cannot change because shouldUpdateReactComponent doesn't allow + // it. TODO: Remove this._owner completely. + this._owner = nextParentElement._owner; + + return; } - } - if (shouldUpdate) { this._pendingForceUpdate = false; // Will set `this.props`, `this.state` and `this.context`. this._performComponentUpdate( - nextElement, + nextParentElement, nextProps, nextState, nextContext, transaction ); - } else { - // If it's determined that a component should not update, we still want - // to set props and state. - this._currentElement = nextElement; - this.props = nextProps; - this.state = nextState; - this.context = nextContext; - - // Owner cannot change because shouldUpdateReactComponent doesn't allow - // it. TODO: Remove this._owner completely. - this._owner = nextElement._owner; - } - }, + }), /** * Merges new props and state, notifies delegate methods of update and @@ -1105,7 +1168,6 @@ var ReactCompositeComponentMixin = { nextContext, transaction ) { - var prevElement = this._currentElement; var prevProps = this.props; var prevState = this.state; var prevContext = this.context; @@ -1123,10 +1185,7 @@ var ReactCompositeComponentMixin = { // it. TODO: Remove this._owner completely. this._owner = nextElement._owner; - this.updateComponent( - transaction, - prevElement - ); + this._updateRenderedComponent(transaction); if (this.componentDidUpdate) { transaction.getReactMountReady().enqueue( @@ -1136,73 +1195,41 @@ var ReactCompositeComponentMixin = { } }, - receiveComponent: function(nextElement, transaction) { - if (nextElement === this._currentElement && - nextElement._owner != null) { - // Since elements are immutable after the owner is rendered, - // we can do a cheap identity compare here to determine if this is a - // superfluous reconcile. It's possible for state to be mutable but such - // change should trigger an update of the owner which would recreate - // the element. We explicitly check for the existence of an owner since - // it's possible for a element created outside a composite to be - // deeply mutated and reused. - return; - } - - ReactComponent.Mixin.receiveComponent.call( - this, - nextElement, - transaction - ); - }, - /** - * Updates the component's currently mounted DOM representation. - * - * By default, this implements React's rendering and reconciliation algorithm. - * Sophisticated clients may wish to override this. + * Call the component's `render` method and update the DOM accordingly. * * @param {ReactReconcileTransaction} transaction - * @param {ReactElement} prevElement * @internal - * @overridable */ - updateComponent: ReactPerf.measure( - 'ReactCompositeComponent', - 'updateComponent', - function(transaction, prevParentElement) { - ReactComponent.Mixin.updateComponent.call( - this, + _updateRenderedComponent: function(transaction) { + var prevComponentInstance = this._renderedComponent; + var prevRenderedElement = prevComponentInstance._currentElement; + var nextRenderedElement = this._renderValidatedComponent(); + if (shouldUpdateReactComponent(prevRenderedElement, nextRenderedElement)) { + prevComponentInstance.receiveComponent( + nextRenderedElement, + transaction + ); + } else { + // These two IDs are actually the same! But nothing should rely on that. + var thisID = this._rootNodeID; + var prevComponentID = prevComponentInstance._rootNodeID; + prevComponentInstance.unmountComponent(); + this._renderedComponent = instantiateReactComponent( + nextRenderedElement, + this._currentElement.type + ); + var nextMarkup = this._renderedComponent.mountComponent( + thisID, transaction, - prevParentElement + this._mountDepth + 1 + ); + ReactComponent.BackendIDOperations.dangerouslyReplaceNodeWithMarkupByID( + prevComponentID, + nextMarkup ); - - var prevComponentInstance = this._renderedComponent; - var prevElement = prevComponentInstance._currentElement; - var nextElement = this._renderValidatedComponent(); - if (shouldUpdateReactComponent(prevElement, nextElement)) { - prevComponentInstance.receiveComponent(nextElement, transaction); - } else { - // These two IDs are actually the same! But nothing should rely on that. - var thisID = this._rootNodeID; - var prevComponentID = prevComponentInstance._rootNodeID; - prevComponentInstance.unmountComponent(); - this._renderedComponent = instantiateReactComponent( - nextElement, - this._currentElement.type - ); - var nextMarkup = this._renderedComponent.mountComponent( - thisID, - transaction, - this._mountDepth + 1 - ); - ReactComponent.BackendIDOperations.dangerouslyReplaceNodeWithMarkupByID( - prevComponentID, - nextMarkup - ); - } } - ), + }, /** * Forces an update. This should only be invoked when it is known with diff --git a/src/core/__tests__/ReactCompositeComponent-test.js b/src/core/__tests__/ReactCompositeComponent-test.js index bfe8e42371..cf253ffc33 100644 --- a/src/core/__tests__/ReactCompositeComponent-test.js +++ b/src/core/__tests__/ReactCompositeComponent-test.js @@ -1483,4 +1483,40 @@ describe('ReactCompositeComponent', function() { ); }); + it('should update refs if shouldComponentUpdate gives false', function() { + var Static = React.createClass({ + shouldComponentUpdate: function() { + return false; + }, + render: function() { + return
{this.props.children}
; + } + }); + var Component = React.createClass({ + render: function() { + if (this.props.flipped) { + return
+ B (ignored) + A (ignored) +
; + } else { + return
+ A + B +
; + } + } + }); + + var comp = ReactTestUtils.renderIntoDocument(); + expect(comp.refs.static0.getDOMNode().textContent).toBe('A'); + expect(comp.refs.static1.getDOMNode().textContent).toBe('B'); + + // When flipping the order, the refs should update even though the actual + // contents do not + comp.setProps({flipped: true}); + expect(comp.refs.static0.getDOMNode().textContent).toBe('B'); + expect(comp.refs.static1.getDOMNode().textContent).toBe('A'); + }); + });