Fix use of stale props in Fabric events (#26408)
## Summary We had to revert the last React sync to React Native because we saw issues with Responder events using stale event handlers instead of recent versions. I reviewed the merged PRs and realized the problem was in the refactor I did in #26321. In that PR, we moved `currentProps` from `canonical`, which is a singleton referenced by all versions of the same fiber, to the fiber itself. This is causing the staleness we observed in events. This PR does a partial revert of the refactor in #26321, bringing back the `canonical` object but moving `publicInstance` to one of its fields, instead of being the `canonical` object itself. ## How did you test this change? Existing unit tests continue working (I didn't manage to get a repro using the test renderer). I manually tested this change in Meta infra and saw the problem was fixed.
Rubén Norte committed
Mar 17, 2023 at 11:27 UTC
108aed083e6f53c14d2b87cbd960fe3fb53fdb37
6 files changed
+47
-41
packages/react-native-renderer/src/ReactFabricComponentTree.js
+6
-3
@@ -22,8 +22,11 @@ import {getPublicInstance} from './ReactFabricHostConfig';
22
function getInstanceFromNode(node: Instance | TextInstance): Fiber | null {
23
const instance: Instance = (node: $FlowFixMe); // In React Native, node is never a text instance
24
25
- if (instance.internalInstanceHandle != null) {
26
- return instance.internalInstanceHandle;
25
+ if (
26
+ instance.canonical != null &&
27
+ instance.canonical.internalInstanceHandle != null
28
+ ) {
29
+ return instance.canonical.internalInstanceHandle;
30
}
31
32
// $FlowFixMe[incompatible-return] DevTools incorrectly passes a fiber in React Native.
@@ -41,7 +44,7 @@ function getNodeFromInstance(fiber: Fiber): PublicInstance {
44
}
45
46
function getFiberCurrentPropsFromNode(instance: Instance): Props {
44
- return instance.currentProps;
47
+ return instance.canonical.currentProps;
48
}
49
50
export {
packages/react-native-renderer/src/ReactFabricHostConfig.js
+23
-27
@@ -55,13 +55,15 @@ export type Props = Object;
55
export type Instance = {
56
// Reference to the shadow node.
57
node: Node,
58
- nativeTag: number,
59
- viewConfig: ViewConfig,
60
- currentProps: Props,
61
- // Reference to the React handle (the fiber)
62
- internalInstanceHandle: Object,
63
- // Exposed through refs.
64
- publicInstance: ReactFabricHostComponent,
58
+ canonical: {
59
+ nativeTag: number,
60
+ viewConfig: ViewConfig,
61
+ currentProps: Props,
62
+ // Reference to the React handle (the fiber)
63
+ internalInstanceHandle: Object,
64
+ // Exposed through refs.
65
+ publicInstance: ReactFabricHostComponent,
66
+ },
67
};
68
export type TextInstance = {node: Node, ...};
69
export type HydratableInstance = Instance | TextInstance;
@@ -148,11 +150,13 @@ export function createInstance(
150
151
return {
152
node: node,
151
- nativeTag: tag,
152
- viewConfig,
153
- currentProps: props,
154
- internalInstanceHandle,
155
- publicInstance: component,
153
+ canonical: {
154
+ nativeTag: tag,
155
+ viewConfig,
156
+ currentProps: props,
157
+ internalInstanceHandle,
158
+ publicInstance: component,
159
+ },
160
};
161
}
162
@@ -222,8 +226,8 @@ export function getChildHostContext(
226
}
227
228
export function getPublicInstance(instance: Instance): null | PublicInstance {
225
- if (instance.publicInstance != null) {
226
- return instance.publicInstance;
229
+ if (instance.canonical != null && instance.canonical.publicInstance != null) {
230
+ return instance.canonical.publicInstance;
231
}
232
233
// For compatibility with the legacy renderer, in case it's used with Fabric
@@ -249,12 +253,12 @@ export function prepareUpdate(
253
newProps: Props,
254
hostContext: HostContext,
255
): null | Object {
252
- const viewConfig = instance.viewConfig;
256
+ const viewConfig = instance.canonical.viewConfig;
257
const updatePayload = diff(oldProps, newProps, viewConfig.validAttributes);
258
// TODO: If the event handlers have changed, we need to update the current props
259
// in the commit phase but there is no host config hook to do it yet.
260
// So instead we hack it by updating it in the render phase.
257
- instance.currentProps = newProps;
261
+ instance.canonical.currentProps = newProps;
262
return updatePayload;
263
}
264
@@ -333,11 +337,7 @@ export function cloneInstance(
337
}
338
return {
339
node: clone,
336
- nativeTag: instance.nativeTag,
337
- viewConfig: instance.viewConfig,
338
- currentProps: instance.currentProps,
339
- internalInstanceHandle: instance.internalInstanceHandle,
340
- publicInstance: instance.publicInstance,
340
+ canonical: instance.canonical,
341
};
342
}
343
@@ -347,7 +347,7 @@ export function cloneHiddenInstance(
347
props: Props,
348
internalInstanceHandle: Object,
349
): Instance {
350
- const viewConfig = instance.viewConfig;
350
+ const viewConfig = instance.canonical.viewConfig;
351
const node = instance.node;
352
const updatePayload = create(
353
{style: {display: 'none'}},
@@ -355,11 +355,7 @@ export function cloneHiddenInstance(
355
);
356
return {
357
node: cloneNodeWithNewProps(node, updatePayload),
358
- nativeTag: instance.nativeTag,
359
- viewConfig: instance.viewConfig,
360
- currentProps: instance.currentProps,
361
- internalInstanceHandle: instance.internalInstanceHandle,
362
- publicInstance: instance.publicInstance,
358
+ canonical: instance.canonical,
359
};
360
}
361
packages/react-native-renderer/src/ReactNativeComponentTree.js
+3
-3
@@ -24,10 +24,10 @@ function getInstanceFromTag(tag) {
24
function getTagFromInstance(inst) {
25
let nativeInstance = inst.stateNode;
26
let tag = nativeInstance._nativeTag;
27
- if (tag === undefined) {
27
+ if (tag === undefined && nativeInstance.canonical != null) {
28
// For compatibility with Fabric
29
- tag = nativeInstance.nativeTag;
30
- nativeInstance = nativeInstance.publicInstance;
29
+ tag = nativeInstance.canonical.nativeTag;
30
+ nativeInstance = nativeInstance.canonical.publicInstance;
31
}
32
33
if (!tag) {
packages/react-native-renderer/src/ReactNativeFiberInspector.js
+3
-2
@@ -223,10 +223,11 @@ function getInspectorDataForViewAtPoint(
223
}
224
225
closestInstance =
226
- internalInstanceHandle.stateNode.internalInstanceHandle;
226
+ internalInstanceHandle.stateNode.canonical.internalInstanceHandle;
227
228
// Note: this is deprecated and we want to remove it ASAP. Keeping it here for React DevTools compatibility for now.
229
- const nativeViewTag = internalInstanceHandle.stateNode.nativeTag;
229
+ const nativeViewTag =
230
+ internalInstanceHandle.stateNode.canonical.nativeTag;
231
232
nativeFabricUIManager.measure(
233
node,
packages/react-native-renderer/src/ReactNativeHostConfig.js
+2
-2
@@ -218,8 +218,8 @@ export function getChildHostContext(
218
219
export function getPublicInstance(instance: Instance): * {
220
// $FlowExpectedError[prop-missing] For compatibility with Fabric
221
- if (instance.publicInstance != null) {
222
- return instance.publicInstance;
221
+ if (instance.canonical != null && instance.canonical.publicInstance != null) {
222
+ return instance.canonical.publicInstance;
223
}
224
225
return instance;
packages/react-native-renderer/src/ReactNativePublicCompat.js
+10
-4
@@ -56,9 +56,12 @@ export function findHostInstance_DEPRECATED<TElementType: ElementType>(
56
}
57
58
// For compatibility with Fabric instances
59
- if (componentOrHandle.publicInstance) {
59
+ if (
60
+ componentOrHandle.canonical &&
61
+ componentOrHandle.canonical.publicInstance
62
+ ) {
63
// $FlowExpectedError[incompatible-return] Can't refine componentOrHandle as a Fabric instance
61
- return componentOrHandle.publicInstance;
64
+ return componentOrHandle.canonical.publicInstance;
65
}
66
67
// For compatibility with legacy renderer instances
@@ -117,8 +120,11 @@ export function findNodeHandle(componentOrHandle: any): ?number {
120
}
121
122
// For compatibility with Fabric instances
120
- if (componentOrHandle.nativeTag != null) {
121
- return componentOrHandle.nativeTag;
123
+ if (
124
+ componentOrHandle.canonical != null &&
125
+ componentOrHandle.canonical.nativeTag != null
126
+ ) {
127
+ return componentOrHandle.canonical.nativeTag;
128
}
129
130
// For compatibility with Fabric public instances