diff --git a/src/core/ReactCompositeComponent.js b/src/core/ReactCompositeComponent.js index 8edbc81ff0..308181025b 100644 --- a/src/core/ReactCompositeComponent.js +++ b/src/core/ReactCompositeComponent.js @@ -30,6 +30,7 @@ var keyMirror = require('keyMirror'); var merge = require('merge'); var mixInto = require('mixInto'); var objMap = require('objMap'); +var shouldUpdateReactComponent = require('shouldUpdateReactComponent'); /** * Policies that describe methods in `ReactCompositeComponentInterface`. @@ -789,15 +790,15 @@ var ReactCompositeComponentMixin = { 'updateComponent', function(transaction, prevProps, prevState) { ReactComponent.Mixin.updateComponent.call(this, transaction, prevProps); - var currentComponent = this._renderedComponent; + var prevComponent = this._renderedComponent; var nextComponent = this._renderValidatedComponent(); - if (currentComponent.constructor === nextComponent.constructor) { - currentComponent.receiveProps(nextComponent.props, transaction); + if (shouldUpdateReactComponent(prevComponent, nextComponent)) { + prevComponent.receiveProps(nextComponent.props, transaction); } else { // These two IDs are actually the same! But nothing should rely on that. var thisID = this._rootNodeID; - var currentComponentID = currentComponent._rootNodeID; - currentComponent.unmountComponent(); + var prevComponentID = prevComponent._rootNodeID; + prevComponent.unmountComponent(); this._renderedComponent = nextComponent; var nextMarkup = nextComponent.mountComponent( thisID, @@ -805,7 +806,7 @@ var ReactCompositeComponentMixin = { this._mountDepth + 1 ); ReactComponent.DOMIDOperations.dangerouslyReplaceNodeWithMarkupByID( - currentComponentID, + prevComponentID, nextMarkup ); } diff --git a/src/core/ReactMount.js b/src/core/ReactMount.js index 29d8b626a9..6ff7378bd5 100644 --- a/src/core/ReactMount.js +++ b/src/core/ReactMount.js @@ -25,6 +25,7 @@ var $ = require('$'); var getReactRootElementInContainer = require('getReactRootElementInContainer'); var invariant = require('invariant'); var nodeContains = require('nodeContains'); +var shouldUpdateReactComponent = require('shouldUpdateReactComponent'); var SEPARATOR = ReactInstanceHandles.SEPARATOR; @@ -304,12 +305,12 @@ var ReactMount = { * @return {ReactComponent} Component instance rendered in `container`. */ renderComponent: function(nextComponent, container, callback) { - var registeredComponent = instancesByReactRootID[getReactRootID(container)]; + var prevComponent = instancesByReactRootID[getReactRootID(container)]; - if (registeredComponent) { - if (registeredComponent.constructor === nextComponent.constructor) { + if (prevComponent) { + if (shouldUpdateReactComponent(prevComponent, nextComponent)) { return ReactMount._updateRootComponent( - registeredComponent, + prevComponent, nextComponent, container, callback @@ -323,7 +324,7 @@ var ReactMount = { var containerHasReactMarkup = reactRootElement && ReactMount.isRenderedByReact(reactRootElement); - var shouldReuseMarkup = containerHasReactMarkup && !registeredComponent; + var shouldReuseMarkup = containerHasReactMarkup && !prevComponent; var component = ReactMount._renderNewRootComponent( nextComponent, diff --git a/src/core/ReactMultiChild.js b/src/core/ReactMultiChild.js index e0e12cd82e..39011119dd 100644 --- a/src/core/ReactMultiChild.js +++ b/src/core/ReactMultiChild.js @@ -23,19 +23,7 @@ var ReactComponent = require('ReactComponent'); var ReactMultiChildUpdateTypes = require('ReactMultiChildUpdateTypes'); var flattenChildren = require('flattenChildren'); - -/** - * Given a `curChild` and `newChild`, determines if `curChild` should be - * updated as opposed to being destroyed or replaced. - * - * @param {?ReactComponent} curChild - * @param {?ReactComponent} newChild - * @return {boolean} True if `curChild` should be updated with `newChild`. - * @protected - */ -function shouldUpdateChild(curChild, newChild) { - return curChild && newChild && curChild.constructor === newChild.constructor; -} +var shouldUpdateReactComponent = require('shouldUpdateReactComponent'); /** * Updating children of a component may trigger recursive updates. The depth is @@ -294,7 +282,7 @@ var ReactMultiChild = { } var prevChild = prevChildren && prevChildren[name]; var nextChild = nextChildren[name]; - if (shouldUpdateChild(prevChild, nextChild)) { + if (shouldUpdateReactComponent(prevChild, nextChild)) { this.moveChild(prevChild, nextIndex, lastIndex); lastIndex = Math.max(prevChild._mountIndex, lastIndex); prevChild.receiveProps(nextChild.props, transaction); diff --git a/src/core/__tests__/ReactMultiChild-test.js b/src/core/__tests__/ReactMultiChild-test.js index d8bb25a165..747cb41533 100644 --- a/src/core/__tests__/ReactMultiChild-test.js +++ b/src/core/__tests__/ReactMultiChild-test.js @@ -17,61 +17,168 @@ * @emails react-core */ +var mocks = require('mocks'); + describe('ReactMultiChild', function() { var React; - var setInnerHTML; - - // Only run this test suite if `Element.prototype.innerHTML` can be spied on. - var innerHTMLDescriptor = Object.getOwnPropertyDescriptor( - Element.prototype, - 'innerHTML' - ); - if (!innerHTMLDescriptor) { - return; - } beforeEach(function() { require('mock-modules').dumpCache(); React = require('React'); + }); - Object.defineProperty(Element.prototype, 'innerHTML', { - set: setInnerHTML = jasmine.createSpy().andCallFake( - innerHTMLDescriptor.set - ) + describe('reconciliation', function() { + it('should update children when possible', function() { + var container = document.createElement('div'); + + var mockMount = mocks.getMockFunction(); + var mockUpdate = mocks.getMockFunction(); + var mockUnmount = mocks.getMockFunction(); + + var MockComponent = React.createClass({ + componentDidMount: mockMount, + componentDidUpdate: mockUpdate, + componentWillUnmount: mockUnmount, + render: function() { + return ; + } + }); + + expect(mockMount.mock.calls.length).toBe(0); + expect(mockUpdate.mock.calls.length).toBe(0); + expect(mockUnmount.mock.calls.length).toBe(0); + + React.renderComponent(
, container); + + expect(mockMount.mock.calls.length).toBe(1); + expect(mockUpdate.mock.calls.length).toBe(0); + expect(mockUnmount.mock.calls.length).toBe(0); + + React.renderComponent(
, container); + + expect(mockMount.mock.calls.length).toBe(1); + expect(mockUpdate.mock.calls.length).toBe(1); + expect(mockUnmount.mock.calls.length).toBe(0); + }); + + it('should replace children with different constructors', function() { + var container = document.createElement('div'); + + var mockMount = mocks.getMockFunction(); + var mockUnmount = mocks.getMockFunction(); + + var MockComponent = React.createClass({ + componentDidMount: mockMount, + componentWillUnmount: mockUnmount, + render: function() { + return ; + } + }); + + expect(mockMount.mock.calls.length).toBe(0); + expect(mockUnmount.mock.calls.length).toBe(0); + + React.renderComponent(
, container); + + expect(mockMount.mock.calls.length).toBe(1); + expect(mockUnmount.mock.calls.length).toBe(0); + + React.renderComponent(
, container); + + expect(mockMount.mock.calls.length).toBe(1); + expect(mockUnmount.mock.calls.length).toBe(1); + }); + + it('should replace children with different owners', function() { + var container = document.createElement('div'); + + var mockMount = mocks.getMockFunction(); + var mockUnmount = mocks.getMockFunction(); + + var MockComponent = React.createClass({ + componentDidMount: mockMount, + componentWillUnmount: mockUnmount, + render: function() { + return ; + } + }); + + var WrapperComponent = React.createClass({ + render: function() { + return this.props.children || ; + } + }); + + expect(mockMount.mock.calls.length).toBe(0); + expect(mockUnmount.mock.calls.length).toBe(0); + + React.renderComponent(, container); + + expect(mockMount.mock.calls.length).toBe(1); + expect(mockUnmount.mock.calls.length).toBe(0); + + React.renderComponent( + , + container + ); + + expect(mockMount.mock.calls.length).toBe(2); + expect(mockUnmount.mock.calls.length).toBe(1); }); }); - it('should only set `innerHTML` once on update', function() { - var container = document.createElement('div'); + describe('innerHTML', function() { + var setInnerHTML; - React.renderComponent( -
-

-

-

-
, - container + // Only run this suite if `Element.prototype.innerHTML` can be spied on. + var innerHTMLDescriptor = Object.getOwnPropertyDescriptor( + Element.prototype, + 'innerHTML' ); - // Warm the cache used by `getMarkupWrap`. - React.renderComponent( -
-

-

-

-
, - container - ); - expect(setInnerHTML).toHaveBeenCalled(); - var callCountOnMount = setInnerHTML.callCount; + if (!innerHTMLDescriptor) { + return; + } - React.renderComponent( -
-

-

-

-
, - container - ); - expect(setInnerHTML.callCount).toBe(callCountOnMount + 1); + beforeEach(function() { + Object.defineProperty(Element.prototype, 'innerHTML', { + set: setInnerHTML = jasmine.createSpy().andCallFake( + innerHTMLDescriptor.set + ) + }); + }); + + it('should only set `innerHTML` once on update', function() { + var container = document.createElement('div'); + + React.renderComponent( +
+

+

+

+
, + container + ); + // Warm the cache used by `getMarkupWrap`. + React.renderComponent( +
+

+

+

+
, + container + ); + expect(setInnerHTML).toHaveBeenCalled(); + var callCountOnMount = setInnerHTML.callCount; + + React.renderComponent( +
+

+

+

+
, + container + ); + expect(setInnerHTML.callCount).toBe(callCountOnMount + 1); + }); }); }); diff --git a/src/core/shouldUpdateReactComponent.js b/src/core/shouldUpdateReactComponent.js new file mode 100644 index 0000000000..77261f2b3d --- /dev/null +++ b/src/core/shouldUpdateReactComponent.js @@ -0,0 +1,53 @@ +/** + * Copyright 2013 Facebook, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + * @providesModule shouldUpdateReactComponent + * @typechecks static-only + */ + +/** + * Given a `prevComponent` and `nextComponent`, determines if `prevComponent` + * should be updated as opposed to being destroyed or replaced. + * + * @param {?object} prevComponent + * @param {?object} nextComponent + * @return {boolean} True if `prevComponent` should be updated. + * @protected + */ +function shouldUpdateReactComponent(prevComponent, nextComponent) { + // TODO: Remove warning after a release. + if (prevComponent && nextComponent && + prevComponent.constructor === nextComponent.constructor) { + if (prevComponent.props.__owner__ === nextComponent.props.__owner__) { + return true; + } else { + if (__DEV__) { + if (prevComponent.state) { + console.warn( + 'A recent change to React has been found to impact your code. ' + + 'A mounted component will now be unmounted and replaced by a ' + + 'component (of the same class) if their owners are different. ' + + 'Previously, ownership was not considered when updating.', + prevComponent, + nextComponent + ); + } + } + } + } + return false; +} + +module.exports = shouldUpdateReactComponent;