facebook
Preserve state when cloning the shadow tree using `cloneTree` method
Summary:
The current behavior when calling cloneTree is to not copy the state to the ShadowNodeFragment which results in the state being equal to statePlaceholder. Inside the relevant ShadowNode constructor, the state is checked and if no state was passed, the most recent mounted one is used for the new node: https://github.com/facebook/react-native/blob/a1e81185416a53c7c7d0cfc67e40079fd0073e7c/packages/react-native/ReactCommon/react/renderer/core/ShadowNode.cpp#L101-L103
mostRecentState is updated during the first construction of a node in the family: https://github.com/facebook/react-native/blob/a1e81185416a53c7c7d0cfc67e40079fd0073e7c/packages/react-native/ReactCommon/react/renderer/core/ShadowNode.cpp#L88
and when the new node is mounted: https://github.com/facebook/react-native/blob/a1e81185416a53c7c7d0cfc67e40079fd0073e7c/packages/react-native/ReactCommon/react/renderer/core/ShadowNode.cpp#L282-L289
This turns out to be a problem when using custom commit hooks which rely on the cloneTree method. Let's take the following case:
graph TD
A((A))
B((B))
C((C))
D((D))
A --> B
B --> C
C --> D
where node B has a custom native state. Now let's say there is also a commit hook that will modify the node D by calling cloneTree with its family as the argument.
In case there is a commit that would modify the native state of B. tryCommit method would be called and would generate a new tree where B has the new state. The commit hook would also trigger on the new tree resulting in the entire tree being cloned to modify the D node in some way. The problem is that it would clone the entire path to D (so A, B, and C) without setting the state in the fragment used to clone them, because of that the state used to clone them the second time would be the most recently committed one. Now, the new state for B node hasn't yet been committed as it happens on mount, so it wouldn't be used when cloning. This effectively means that the state update for a node is dropped when a commit hook tries to modify any of its descendants using the cloneTree method.
Change from this PR makes it so the state from the tree being cloned is used when cloning nodes instead of the most recently committed one. That behavior seems to make more sense to me since potentially modifying the state of nodes on the path to the actually cloned one seems like an unwanted side effect.
The same change has been already rolled out in Reanimated's customized cloning logic (for similar reasons) and we didn't observe any problems caused by it.
Related issue: https://github.com/Expensify/react-native-live-markdown/issues/346
Changelog:
[GENERAL] [FIXED] - Preserve state from the shadow tree being cloned when calling cloneTree
Test Plan:
I've tried it out on RNTestes and didn't see anything out of the ordinary, though I'm not sure whether this change may have some unintended consequences.
| Platform | Engine | Arch | Size (bytes) | Diff |
|---|---|---|---|---|
| android | hermes | arm64-v8a | 19,576,603 | -98,422 |
| android | hermes | armeabi-v7a | n/a | -- |
| android | hermes | x86 | n/a | -- |
| android | hermes | x86_64 | n/a | -- |
| android | jsc | arm64-v8a | 22,946,176 | -98,346 |
| android | jsc | armeabi-v7a | n/a | -- |
| android | jsc | x86 | n/a | -- |
| android | jsc | x86_64 | n/a | -- |
Base commit: a1e81185416a53c7c7d0cfc67e40079fd0073e7c Branch: main
@mdvacca has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.
This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.
This PR was closed because it has been stalled for 7 days with no activity.