diff --git a/packages/react-dom/src/__tests__/ReactDOMFiber-test.js b/packages/react-dom/src/__tests__/ReactDOMFiber-test.js index 18e95407a8936..a28be568f7d86 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFiber-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFiber-test.js @@ -18,6 +18,12 @@ describe('ReactDOMFiber', () => { beforeEach(() => { container = document.createElement('div'); + document.body.appendChild(container); + }); + + afterEach(() => { + document.body.removeChild(container); + container = null; }); it('should render strings as children', () => { @@ -205,12 +211,12 @@ describe('ReactDOMFiber', () => { }; const assertNamespacesMatch = function(tree) { - container = document.createElement('div'); + let testContainer = document.createElement('div'); svgEls = []; htmlEls = []; mathEls = []; - ReactDOM.render(tree, container); + ReactDOM.render(tree, testContainer); svgEls.forEach(el => { expect(el.namespaceURI).toBe('http://www.w3.org/2000/svg'); }); @@ -221,8 +227,8 @@ describe('ReactDOMFiber', () => { expect(el.namespaceURI).toBe('http://www.w3.org/1998/Math/MathML'); }); - ReactDOM.unmountComponentAtNode(container); - expect(container.innerHTML).toBe(''); + ReactDOM.unmountComponentAtNode(testContainer); + expect(testContainer.innerHTML).toBe(''); }; it('should render one portal', () => { @@ -874,7 +880,6 @@ describe('ReactDOMFiber', () => { it('should not onMouseLeave when staying in the portal', () => { const portalContainer = document.createElement('div'); - document.body.appendChild(container); document.body.appendChild(portalContainer); let ops = []; @@ -944,7 +949,6 @@ describe('ReactDOMFiber', () => { 'leave parent', // Only when we leave the portal does onMouseLeave fire. ]); } finally { - document.body.removeChild(container); document.body.removeChild(portalContainer); } }); @@ -987,82 +991,77 @@ describe('ReactDOMFiber', () => { }); it('should not update event handlers until commit', () => { - document.body.appendChild(container); - try { - let ops = []; - const handlerA = () => ops.push('A'); - const handlerB = () => ops.push('B'); - - class Example extends React.Component { - state = {flip: false, count: 0}; - flip() { - this.setState({flip: true, count: this.state.count + 1}); - } - tick() { - this.setState({count: this.state.count + 1}); - } - render() { - const useB = !this.props.forceA && this.state.flip; - return
; - } + let ops = []; + const handlerA = () => ops.push('A'); + const handlerB = () => ops.push('B'); + + class Example extends React.Component { + state = {flip: false, count: 0}; + flip() { + this.setState({flip: true, count: this.state.count + 1}); } + tick() { + this.setState({count: this.state.count + 1}); + } + render() { + const useB = !this.props.forceA && this.state.flip; + return
; + } + } - class Click extends React.Component { - constructor() { - super(); - node.click(); - } - render() { - return null; - } + class Click extends React.Component { + constructor() { + super(); + node.click(); } + render() { + return null; + } + } - let inst; - ReactDOM.render([ (inst = n)} />], container); - const node = container.firstChild; - expect(node.tagName).toEqual('DIV'); + let inst; + ReactDOM.render([ (inst = n)} />], container); + const node = container.firstChild; + expect(node.tagName).toEqual('DIV'); - node.click(); + node.click(); - expect(ops).toEqual(['A']); - ops = []; + expect(ops).toEqual(['A']); + ops = []; - // Render with the other event handler. - inst.flip(); + // Render with the other event handler. + inst.flip(); - node.click(); + node.click(); - expect(ops).toEqual(['B']); - ops = []; + expect(ops).toEqual(['B']); + ops = []; - // Rerender without changing any props. - inst.tick(); + // Rerender without changing any props. + inst.tick(); - node.click(); + node.click(); - expect(ops).toEqual(['B']); - ops = []; + expect(ops).toEqual(['B']); + ops = []; - // Render a flip back to the A handler. The second component invokes the - // click handler during render to simulate a click during an aborted - // render. I use this hack because at current time we don't have a way to - // test aborted ReactDOM renders. - ReactDOM.render( - [, ], - container, - ); + // Render a flip back to the A handler. The second component invokes the + // click handler during render to simulate a click during an aborted + // render. I use this hack because at current time we don't have a way to + // test aborted ReactDOM renders. + ReactDOM.render( + [, ], + container, + ); - // Because the new click handler has not yet committed, we should still - // invoke B. - expect(ops).toEqual(['B']); - ops = []; + // Because the new click handler has not yet committed, we should still + // invoke B. + expect(ops).toEqual(['B']); + ops = []; - // Any click that happens after commit, should invoke A. - node.click(); - expect(ops).toEqual(['A']); - } finally { - document.body.removeChild(container); - } + // Any click that happens after commit, should invoke A. + node.click(); + expect(ops).toEqual(['A']); }); it('should not crash encountering low-priority tree', () => { @@ -1178,4 +1177,63 @@ describe('ReactDOMFiber', () => { container.appendChild(fragment); expect(container.innerHTML).toBe('
foo
'); }); + + // Regression test for https://github.com/facebook/react/issues/12643#issuecomment-413727104 + it('should not diff memoized host components', () => { + let inputRef = React.createRef(); + let didCallOnChange = false; + + class Child extends React.Component { + state = {}; + componentDidMount() { + document.addEventListener('click', this.update, true); + } + componentWillUnmount() { + document.removeEventListener('click', this.update, true); + } + update = () => { + // We're testing that this setState() + // doesn't cause React to commit updates + // to the input outside (which would itself + // prevent the parent's onChange parent handler + // from firing). + this.setState({}); + // Note that onChange was always broken when there was an + // earlier setState() in a manual document capture phase + // listener *in the same component*. But that's very rare. + // Here we're testing that a *child* component doesn't break + // the parent if this happens. + }; + render() { + return
; + } + } + + class Parent extends React.Component { + handleChange = val => { + didCallOnChange = true; + }; + render() { + return ( +
+ + +
+ ); + } + } + + ReactDOM.render(, container); + inputRef.current.dispatchEvent( + new MouseEvent('click', { + bubbles: true, + }), + ); + expect(didCallOnChange).toBe(true); + }); }); diff --git a/packages/react-noop-renderer/src/createReactNoop.js b/packages/react-noop-renderer/src/createReactNoop.js index 81f2081f2fe4a..98da3b8053b73 100644 --- a/packages/react-noop-renderer/src/createReactNoop.js +++ b/packages/react-noop-renderer/src/createReactNoop.js @@ -41,6 +41,8 @@ if (__DEV__) { function createReactNoop(reconciler: Function, useMutation: boolean) { let scheduledCallback = null; let instanceCounter = 0; + let hostDiffCounter = 0; + let hostUpdateCounter = 0; function appendChildToContainerOrInstance( parentInstance: Container | Instance, @@ -220,6 +222,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { if (newProps === null) { throw new Error('Should have new props'); } + hostDiffCounter++; return UPDATE_SIGNAL; }, @@ -303,6 +306,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { if (oldProps === null) { throw new Error('Should have old props'); } + hostUpdateCounter++; instance.prop = newProps.prop; }, @@ -311,6 +315,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { oldText: string, newText: string, ): void { + hostUpdateCounter++; textInstance.text = newText; }, @@ -556,6 +561,26 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { return actual !== null ? actual : []; }, + flushWithHostCounters( + fn: () => void, + ): {| + hostDiffCounter: number, + hostUpdateCounter: number, + |} { + hostDiffCounter = 0; + hostUpdateCounter = 0; + try { + ReactNoop.flush(); + return { + hostDiffCounter, + hostUpdateCounter, + }; + } finally { + hostDiffCounter = 0; + hostUpdateCounter = 0; + } + }, + expire(ms: number): Array { ReactNoop.advanceTime(ms); return ReactNoop.flushExpired(); diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.js b/packages/react-reconciler/src/ReactFiberCompleteWork.js index c20d0e8ad9b35..feef7e4d84ce0 100644 --- a/packages/react-reconciler/src/ReactFiberCompleteWork.js +++ b/packages/react-reconciler/src/ReactFiberCompleteWork.js @@ -361,34 +361,36 @@ function completeWork( // If we have an alternate, that means this is an update and we need to // schedule a side-effect to do the updates. const oldProps = current.memoizedProps; - // If we get updated because one of our children updated, we don't - // have newProps so we'll have to reuse them. - // TODO: Split the update API as separate for the props vs. children. - // Even better would be if children weren't special cased at all tho. - const instance: Instance = workInProgress.stateNode; - const currentHostContext = getHostContext(); - // TODO: Experiencing an error where oldProps is null. Suggests a host - // component is hitting the resume path. Figure out why. Possibly - // related to `hidden`. - const updatePayload = prepareUpdate( - instance, - type, - oldProps, - newProps, - rootContainerInstance, - currentHostContext, - ); + if (oldProps !== newProps) { + // If we get updated because one of our children updated, we don't + // have newProps so we'll have to reuse them. + // TODO: Split the update API as separate for the props vs. children. + // Even better would be if children weren't special cased at all tho. + const instance: Instance = workInProgress.stateNode; + const currentHostContext = getHostContext(); + // TODO: Experiencing an error where oldProps is null. Suggests a host + // component is hitting the resume path. Figure out why. Possibly + // related to `hidden`. + const updatePayload = prepareUpdate( + instance, + type, + oldProps, + newProps, + rootContainerInstance, + currentHostContext, + ); - updateHostComponent( - current, - workInProgress, - updatePayload, - type, - oldProps, - newProps, - rootContainerInstance, - currentHostContext, - ); + updateHostComponent( + current, + workInProgress, + updatePayload, + type, + oldProps, + newProps, + rootContainerInstance, + currentHostContext, + ); + } if (current.ref !== workInProgress.ref) { markRef(workInProgress); diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalUpdatesMinimalism-test.js b/packages/react-reconciler/src/__tests__/ReactIncrementalUpdatesMinimalism-test.js new file mode 100644 index 0000000000000..4b52e6e77f52a --- /dev/null +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalUpdatesMinimalism-test.js @@ -0,0 +1,156 @@ +/** + * Copyright (c) 2013-present, Facebook, Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @emails react-core + * @jest-environment node + */ + +'use strict'; + +let React; +let ReactNoop; + +describe('ReactIncrementalUpdatesMinimalism', () => { + beforeEach(() => { + jest.resetModules(); + React = require('react'); + ReactNoop = require('react-noop-renderer'); + }); + + it('should render a simple component', () => { + function Child() { + return
Hello World
; + } + + function Parent() { + return ; + } + + ReactNoop.render(); + expect(ReactNoop.flushWithHostCounters()).toEqual({ + hostDiffCounter: 0, + hostUpdateCounter: 0, + }); + + ReactNoop.render(); + expect(ReactNoop.flushWithHostCounters()).toEqual({ + hostDiffCounter: 1, + hostUpdateCounter: 1, + }); + }); + + it('should not diff referentially equal host elements', () => { + function Leaf(props) { + return ( + + hello + + {props.name} + + ); + } + + const constEl = ( +
+ +
+ ); + + function Child() { + return constEl; + } + + function Parent() { + return ; + } + + ReactNoop.render(); + expect(ReactNoop.flushWithHostCounters()).toEqual({ + hostDiffCounter: 0, + hostUpdateCounter: 0, + }); + + ReactNoop.render(); + expect(ReactNoop.flushWithHostCounters()).toEqual({ + hostDiffCounter: 0, + hostUpdateCounter: 0, + }); + }); + + it('should not diff parents of setState targets', () => { + let childInst; + + function Leaf(props) { + return ( + + hello + + {props.name} + + ); + } + + class Child extends React.Component { + state = {name: 'Batman'}; + render() { + childInst = this; + return ( +
+ +
+ ); + } + } + + function Parent() { + return ( +
+
+ + +
+ +
+
+ ); + } + + ReactNoop.render(); + expect(ReactNoop.flushWithHostCounters()).toEqual({ + hostDiffCounter: 0, + hostUpdateCounter: 0, + }); + + childInst.setState({name: 'Robin'}); + expect(ReactNoop.flushWithHostCounters()).toEqual({ + // Child > div + // Child > Leaf > span + // Child > Leaf > span > b + hostDiffCounter: 3, + // Child > div + // Child > Leaf > span + // Child > Leaf > span > b + // Child > Leaf > span > #text + hostUpdateCounter: 4, + }); + + ReactNoop.render(); + expect(ReactNoop.flushWithHostCounters()).toEqual({ + // Parent > section + // Parent > section > div + // Parent > section > div > Leaf > span + // Parent > section > div > Leaf > span > b + // Parent > section > div > Child > div + // Parent > section > div > Child > div > Leaf > span + // Parent > section > div > Child > div > Leaf > span > b + // Parent > section > div > hr + // Parent > section > div > Leaf > span + // Parent > section > div > Leaf > span > b + hostDiffCounter: 10, + hostUpdateCounter: 10, + }); + }); +}); diff --git a/packages/react-reconciler/src/__tests__/__snapshots__/ReactIncrementalPerf-test.internal.js.snap b/packages/react-reconciler/src/__tests__/__snapshots__/ReactIncrementalPerf-test.internal.js.snap index f6c7cb67b627b..d725bf9bb0e37 100644 --- a/packages/react-reconciler/src/__tests__/__snapshots__/ReactIncrementalPerf-test.internal.js.snap +++ b/packages/react-reconciler/src/__tests__/__snapshots__/ReactIncrementalPerf-test.internal.js.snap @@ -85,8 +85,8 @@ exports[`ReactDebugFiberPerf deduplicates lifecycle names during commit to reduc ⚛ (Committing Changes) ⚛ (Committing Snapshot Effects: 0 Total) - ⚛ (Committing Host Effects: 3 Total) - ⚛ (Calling Lifecycle Methods: 3 Total) + ⚛ (Committing Host Effects: 2 Total) + ⚛ (Calling Lifecycle Methods: 2 Total) ⚛ B.componentDidUpdate " `; @@ -257,8 +257,8 @@ exports[`ReactDebugFiberPerf measures deprioritized work 1`] = ` ⚛ (Committing Changes) ⚛ (Committing Snapshot Effects: 0 Total) - ⚛ (Committing Host Effects: 3 Total) - ⚛ (Calling Lifecycle Methods: 2 Total) + ⚛ (Committing Host Effects: 1 Total) + ⚛ (Calling Lifecycle Methods: 0 Total) " `; @@ -311,8 +311,8 @@ exports[`ReactDebugFiberPerf recovers from caught errors 1`] = ` ⚛ (Committing Changes) ⚛ (Committing Snapshot Effects: 0 Total) - ⚛ (Committing Host Effects: 2 Total) - ⚛ (Calling Lifecycle Methods: 1 Total) + ⚛ (Committing Host Effects: 1 Total) + ⚛ (Calling Lifecycle Methods: 0 Total) " `; @@ -357,8 +357,8 @@ exports[`ReactDebugFiberPerf skips parents during setState 1`] = ` ⚛ (Committing Changes) ⚛ (Committing Snapshot Effects: 0 Total) - ⚛ (Committing Host Effects: 6 Total) - ⚛ (Calling Lifecycle Methods: 6 Total) + ⚛ (Committing Host Effects: 2 Total) + ⚛ (Calling Lifecycle Methods: 2 Total) " `; @@ -431,8 +431,8 @@ exports[`ReactDebugFiberPerf warns on cascading renders from setState 1`] = ` ⚛ (Committing Changes) ⚛ (Committing Snapshot Effects: 0 Total) - ⚛ (Committing Host Effects: 2 Total) - ⚛ (Calling Lifecycle Methods: 2 Total) + ⚛ (Committing Host Effects: 1 Total) + ⚛ (Calling Lifecycle Methods: 1 Total) " `;