Skip to content

Commit

Permalink
Fix host bailout for the persistent mode (facebook#13611)
Browse files Browse the repository at this point in the history 
* 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
  • Loading branch information
@gaearon @Simek
gaearon authored and Simek committed Oct 25, 2018
1 parent 5698e23 commit a0a7ba1
Show file tree
Hide file tree
Showing 5 changed files with 353 additions and 83 deletions.
39 changes: 39 additions & 0 deletions packages/react-native-renderer/src/__tests__/ReactFabric-test.internal.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
<View>{chars.map(text => <View key={text} title={text} />)}</View>
);
}
}

const ref = React.createRef();
// Wrap in a host node.
ReactFabric.render(
<View>
<Component ref={ref} />
</View>,
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 {
Expand Down
52 changes: 52 additions & 0 deletions packages/react-native-renderer/src/__tests__/__snapshots__/ReactFabric-test.internal.js.snap
View file
Original file line number Diff line number Diff line change
@@ -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
Expand Down
30 changes: 22 additions & 8 deletions packages/react-noop-renderer/src/createReactNoop.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -370,6 +371,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
value: clone.id,
enumerable: false,
});
hostCloneCounter++;
return clone;
},

Expand Down Expand Up @@ -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;
}
},

Expand Down
158 changes: 83 additions & 75 deletions packages/react-reconciler/src/ReactFiberCompleteWork.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,8 @@ import type {
Instance,
Type,
Props,
UpdatePayload,
Container,
ChildSet,
HostContext,
} from './ReactFiberHostConfig';

import {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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
};
Expand Down Expand Up @@ -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);
Expand Down
Loading

0 comments on commit a0a7ba1

Please sign in to comment.