From a0a7ba1ec2e3def99b81f4bd390e777a02af3bcf Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Mon, 10 Sep 2018 19:05:40 +0100 Subject: [PATCH] Fix host bailout for the persistent mode (#13611) * Add regression test for persistent bailout bug * Fork more logic into updateHostComponent This is mostly copy paste. But I added a bailout only to mutation mode. Persistent mode doesn't have that props equality bailout anymore, so the Fabric test now passes. * Add failing test for persistence host minimalism * Add bailouts to the persistent host updates --- .../__tests__/ReactFabric-test.internal.js | 39 +++++ .../ReactFabric-test.internal.js.snap | 52 ++++++ .../src/createReactNoop.js | 30 +++- .../src/ReactFiberCompleteWork.js | 158 +++++++++--------- .../ReactPersistentUpdatesMinimalism-test.js | 157 +++++++++++++++++ 5 files changed, 353 insertions(+), 83 deletions(-) create mode 100644 packages/react-reconciler/src/__tests__/ReactPersistentUpdatesMinimalism-test.js diff --git a/packages/react-native-renderer/src/__tests__/ReactFabric-test.internal.js b/packages/react-native-renderer/src/__tests__/ReactFabric-test.internal.js index 19325de7f9bbb..d81c084af3eb3 100644 --- a/packages/react-native-renderer/src/__tests__/ReactFabric-test.internal.js +++ b/packages/react-native-renderer/src/__tests__/ReactFabric-test.internal.js @@ -237,6 +237,45 @@ describe('ReactFabric', () => { expect(FabricUIManager.__dumpHierarchyForJestTestsOnly()).toMatchSnapshot(); }); + it('recreates host parents even if only children changed', () => { + const View = createReactNativeComponentClass('RCTView', () => ({ + validAttributes: {title: true}, + uiViewClassName: 'RCTView', + })); + + const before = 'abcdefghijklmnopqrst'; + const after = 'mxhpgwfralkeoivcstzy'; + + class Component extends React.Component { + state = { + chars: before, + }; + render() { + const chars = this.state.chars.split(''); + return ( + {chars.map(text => )} + ); + } + } + + const ref = React.createRef(); + // Wrap in a host node. + ReactFabric.render( + + + , + 11, + ); + expect(FabricUIManager.__dumpHierarchyForJestTestsOnly()).toMatchSnapshot(); + + // Call setState() so that we skip over the top-level host node. + // It should still get recreated despite a bailout. + ref.current.setState({ + chars: after, + }); + expect(FabricUIManager.__dumpHierarchyForJestTestsOnly()).toMatchSnapshot(); + }); + it('calls setState with no arguments', () => { let mockArgs; class Component extends React.Component { diff --git a/packages/react-native-renderer/src/__tests__/__snapshots__/ReactFabric-test.internal.js.snap b/packages/react-native-renderer/src/__tests__/__snapshots__/ReactFabric-test.internal.js.snap index 4a0f4d8cf557a..49ecc42c5065c 100644 --- a/packages/react-native-renderer/src/__tests__/__snapshots__/ReactFabric-test.internal.js.snap +++ b/packages/react-native-renderer/src/__tests__/__snapshots__/ReactFabric-test.internal.js.snap @@ -1,5 +1,57 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP +exports[`ReactFabric recreates host parents even if only children changed 1`] = ` +"11 + RCTView null + RCTView null + RCTView {\\"title\\":\\"a\\"} + RCTView {\\"title\\":\\"b\\"} + RCTView {\\"title\\":\\"c\\"} + RCTView {\\"title\\":\\"d\\"} + RCTView {\\"title\\":\\"e\\"} + RCTView {\\"title\\":\\"f\\"} + RCTView {\\"title\\":\\"g\\"} + RCTView {\\"title\\":\\"h\\"} + RCTView {\\"title\\":\\"i\\"} + RCTView {\\"title\\":\\"j\\"} + RCTView {\\"title\\":\\"k\\"} + RCTView {\\"title\\":\\"l\\"} + RCTView {\\"title\\":\\"m\\"} + RCTView {\\"title\\":\\"n\\"} + RCTView {\\"title\\":\\"o\\"} + RCTView {\\"title\\":\\"p\\"} + RCTView {\\"title\\":\\"q\\"} + RCTView {\\"title\\":\\"r\\"} + RCTView {\\"title\\":\\"s\\"} + RCTView {\\"title\\":\\"t\\"}" +`; + +exports[`ReactFabric recreates host parents even if only children changed 2`] = ` +"11 + RCTView null + RCTView null + RCTView {\\"title\\":\\"m\\"} + RCTView {\\"title\\":\\"x\\"} + RCTView {\\"title\\":\\"h\\"} + RCTView {\\"title\\":\\"p\\"} + RCTView {\\"title\\":\\"g\\"} + RCTView {\\"title\\":\\"w\\"} + RCTView {\\"title\\":\\"f\\"} + RCTView {\\"title\\":\\"r\\"} + RCTView {\\"title\\":\\"a\\"} + RCTView {\\"title\\":\\"l\\"} + RCTView {\\"title\\":\\"k\\"} + RCTView {\\"title\\":\\"e\\"} + RCTView {\\"title\\":\\"o\\"} + RCTView {\\"title\\":\\"i\\"} + RCTView {\\"title\\":\\"v\\"} + RCTView {\\"title\\":\\"c\\"} + RCTView {\\"title\\":\\"s\\"} + RCTView {\\"title\\":\\"t\\"} + RCTView {\\"title\\":\\"z\\"} + RCTView {\\"title\\":\\"y\\"}" +`; + exports[`ReactFabric renders and reorders children 1`] = ` "11 RCTView null diff --git a/packages/react-noop-renderer/src/createReactNoop.js b/packages/react-noop-renderer/src/createReactNoop.js index 1d71bd8ab55d5..f70dc462c6007 100644 --- a/packages/react-noop-renderer/src/createReactNoop.js +++ b/packages/react-noop-renderer/src/createReactNoop.js @@ -46,6 +46,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { let instanceCounter = 0; let hostDiffCounter = 0; let hostUpdateCounter = 0; + let hostCloneCounter = 0; function appendChildToContainerOrInstance( parentInstance: Container | Instance, @@ -370,6 +371,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { value: clone.id, enumerable: false, }); + hostCloneCounter++; return clone; }, @@ -579,21 +581,33 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { flushWithHostCounters( fn: () => void, - ): {| - hostDiffCounter: number, - hostUpdateCounter: number, - |} { + ): + | {| + hostDiffCounter: number, + hostUpdateCounter: number, + |} + | {| + hostDiffCounter: number, + hostCloneCounter: number, + |} { hostDiffCounter = 0; hostUpdateCounter = 0; + hostCloneCounter = 0; try { ReactNoop.flush(); - return { - hostDiffCounter, - hostUpdateCounter, - }; + return useMutation + ? { + hostDiffCounter, + hostUpdateCounter, + } + : { + hostDiffCounter, + hostCloneCounter, + }; } finally { hostDiffCounter = 0; hostUpdateCounter = 0; + hostCloneCounter = 0; } }, diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.js b/packages/react-reconciler/src/ReactFiberCompleteWork.js index 41981886b5d85..cdcf541db39fb 100644 --- a/packages/react-reconciler/src/ReactFiberCompleteWork.js +++ b/packages/react-reconciler/src/ReactFiberCompleteWork.js @@ -14,10 +14,8 @@ import type { Instance, Type, Props, - UpdatePayload, Container, ChildSet, - HostContext, } from './ReactFiberHostConfig'; import { @@ -126,13 +124,36 @@ if (supportsMutation) { updateHostComponent = function( current: Fiber, workInProgress: Fiber, - updatePayload: null | UpdatePayload, type: Type, - oldProps: Props, newProps: Props, rootContainerInstance: Container, - currentHostContext: HostContext, ) { + // 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 (oldProps === newProps) { + // In mutation mode, this is sufficient for a bailout because + // we won't touch this node even if children changed. + return; + } + + // 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, + ); // TODO: Type this specific to this type of component. workInProgress.updateQueue = (updatePayload: any); // If the update payload indicates that there is a change or if there @@ -211,54 +232,70 @@ if (supportsMutation) { updateHostComponent = function( current: Fiber, workInProgress: Fiber, - updatePayload: null | UpdatePayload, type: Type, - oldProps: Props, newProps: Props, rootContainerInstance: Container, - currentHostContext: HostContext, ) { + const currentInstance = current.stateNode; + const oldProps = current.memoizedProps; // If there are no effects associated with this node, then none of our children had any updates. // This guarantees that we can reuse all of them. const childrenUnchanged = workInProgress.firstEffect === null; - const currentInstance = current.stateNode; - if (childrenUnchanged && updatePayload === null) { + if (childrenUnchanged && oldProps === newProps) { // No changes, just reuse the existing instance. // Note that this might release a previous clone. workInProgress.stateNode = currentInstance; - } else { - let recyclableInstance = workInProgress.stateNode; - let newInstance = cloneInstance( - currentInstance, - updatePayload, + return; + } + const recyclableInstance: Instance = workInProgress.stateNode; + const currentHostContext = getHostContext(); + let updatePayload = null; + if (oldProps !== newProps) { + updatePayload = prepareUpdate( + recyclableInstance, type, oldProps, newProps, - workInProgress, - childrenUnchanged, - recyclableInstance, + rootContainerInstance, + currentHostContext, ); - if ( - finalizeInitialChildren( - newInstance, - type, - newProps, - rootContainerInstance, - currentHostContext, - ) - ) { - markUpdate(workInProgress); - } - workInProgress.stateNode = newInstance; - if (childrenUnchanged) { - // If there are no other effects in this tree, we need to flag this node as having one. - // Even though we're not going to use it for anything. - // Otherwise parents won't know that there are new children to propagate upwards. - markUpdate(workInProgress); - } else { - // If children might have changed, we have to add them all to the set. - appendAllChildren(newInstance, workInProgress); - } + } + if (childrenUnchanged && updatePayload === null) { + // No changes, just reuse the existing instance. + // Note that this might release a previous clone. + workInProgress.stateNode = currentInstance; + return; + } + let newInstance = cloneInstance( + currentInstance, + updatePayload, + type, + oldProps, + newProps, + workInProgress, + childrenUnchanged, + recyclableInstance, + ); + if ( + finalizeInitialChildren( + newInstance, + type, + newProps, + rootContainerInstance, + currentHostContext, + ) + ) { + markUpdate(workInProgress); + } + workInProgress.stateNode = newInstance; + if (childrenUnchanged) { + // If there are no other effects in this tree, we need to flag this node as having one. + // Even though we're not going to use it for anything. + // Otherwise parents won't know that there are new children to propagate upwards. + markUpdate(workInProgress); + } else { + // If children might have changed, we have to add them all to the set. + appendAllChildren(newInstance, workInProgress); } }; updateHostText = function( @@ -290,12 +327,9 @@ if (supportsMutation) { updateHostComponent = function( current: Fiber, workInProgress: Fiber, - updatePayload: null | UpdatePayload, type: Type, - oldProps: Props, newProps: Props, rootContainerInstance: Container, - currentHostContext: HostContext, ) { // Noop }; @@ -358,39 +392,13 @@ function completeWork( const rootContainerInstance = getRootHostContainer(); const type = workInProgress.type; if (current !== null && workInProgress.stateNode != null) { - // 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 (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, + type, + newProps, + rootContainerInstance, + ); if (current.ref !== workInProgress.ref) { markRef(workInProgress); diff --git a/packages/react-reconciler/src/__tests__/ReactPersistentUpdatesMinimalism-test.js b/packages/react-reconciler/src/__tests__/ReactPersistentUpdatesMinimalism-test.js new file mode 100644 index 0000000000000..03c9918314385 --- /dev/null +++ b/packages/react-reconciler/src/__tests__/ReactPersistentUpdatesMinimalism-test.js @@ -0,0 +1,157 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * 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 ReactNoopPersistent; + +describe('ReactPersistentUpdatesMinimalism', () => { + beforeEach(() => { + jest.resetModules(); + React = require('react'); + ReactNoopPersistent = require('react-noop-renderer/persistent'); + }); + + it('should render a simple component', () => { + function Child() { + return
Hello World
; + } + + function Parent() { + return ; + } + + ReactNoopPersistent.render(); + expect(ReactNoopPersistent.flushWithHostCounters()).toEqual({ + hostDiffCounter: 0, + hostCloneCounter: 0, + }); + + ReactNoopPersistent.render(); + expect(ReactNoopPersistent.flushWithHostCounters()).toEqual({ + hostDiffCounter: 1, + hostCloneCounter: 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 ; + } + + ReactNoopPersistent.render(); + expect(ReactNoopPersistent.flushWithHostCounters()).toEqual({ + hostDiffCounter: 0, + hostCloneCounter: 0, + }); + + ReactNoopPersistent.render(); + expect(ReactNoopPersistent.flushWithHostCounters()).toEqual({ + hostDiffCounter: 0, + hostCloneCounter: 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 ( +
+
+ + +
+ +
+
+ ); + } + + ReactNoopPersistent.render(); + expect(ReactNoopPersistent.flushWithHostCounters()).toEqual({ + hostDiffCounter: 0, + hostCloneCounter: 0, + }); + + childInst.setState({name: 'Robin'}); + expect(ReactNoopPersistent.flushWithHostCounters()).toEqual({ + // section > div > Child > div + // section > div > Child > Leaf > span + // section > div > Child > Leaf > span > b + hostDiffCounter: 3, + // section + // section > div + // section > div > Child > div + // section > div > Child > Leaf > span + // section > div > Child > Leaf > span > b + hostCloneCounter: 5, + }); + + ReactNoopPersistent.render(); + expect(ReactNoopPersistent.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, + hostCloneCounter: 10, + }); + }); +});