Fix use of stale props in Fabric events (#26408)

rubennorte · web-flow · commit 108aed083e6f · 2023-03-17T11:27:49.000Z
## 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.
diff --git a/packages/react-native-renderer/src/ReactFabricComponentTree.js b/packages/react-native-renderer/src/ReactFabricComponentTree.js
@@ -22,8 +22,11 @@ import {getPublicInstance} from './ReactFabricHostConfig';
 function getInstanceFromNode(node: Instance | TextInstance): Fiber | null {
   const instance: Instance = (node: $FlowFixMe); // In React Native, node is never a text instance
 
-  if (instance.internalInstanceHandle != null) {
-    return instance.internalInstanceHandle;
+  if (
+    instance.canonical != null &&
+    instance.canonical.internalInstanceHandle != null
+  ) {
+    return instance.canonical.internalInstanceHandle;
   }
 
   // $FlowFixMe[incompatible-return] DevTools incorrectly passes a fiber in React Native.
@@ -41,7 +44,7 @@ function getNodeFromInstance(fiber: Fiber): PublicInstance {
 }
 
 function getFiberCurrentPropsFromNode(instance: Instance): Props {
-  return instance.currentProps;
+  return instance.canonical.currentProps;
 }
 
 export {
diff --git a/packages/react-native-renderer/src/ReactFabricHostConfig.js b/packages/react-native-renderer/src/ReactFabricHostConfig.js
@@ -55,13 +55,15 @@ export type Props = Object;
 export type Instance = {
   // Reference to the shadow node.
   node: Node,
-  nativeTag: number,
-  viewConfig: ViewConfig,
-  currentProps: Props,
-  // Reference to the React handle (the fiber)
-  internalInstanceHandle: Object,
-  // Exposed through refs.
-  publicInstance: ReactFabricHostComponent,
+  canonical: {
+    nativeTag: number,
+    viewConfig: ViewConfig,
+    currentProps: Props,
+    // Reference to the React handle (the fiber)
+    internalInstanceHandle: Object,
+    // Exposed through refs.
+    publicInstance: ReactFabricHostComponent,
+  },
 };
 export type TextInstance = {node: Node, ...};
 export type HydratableInstance = Instance | TextInstance;
@@ -148,11 +150,13 @@ export function createInstance(
 
   return {
     node: node,
-    nativeTag: tag,
-    viewConfig,
-    currentProps: props,
-    internalInstanceHandle,
-    publicInstance: component,
+    canonical: {
+      nativeTag: tag,
+      viewConfig,
+      currentProps: props,
+      internalInstanceHandle,
+      publicInstance: component,
+    },
   };
 }
 
@@ -222,8 +226,8 @@ export function getChildHostContext(
 }
 
 export function getPublicInstance(instance: Instance): null | PublicInstance {
-  if (instance.publicInstance != null) {
-    return instance.publicInstance;
+  if (instance.canonical != null && instance.canonical.publicInstance != null) {
+    return instance.canonical.publicInstance;
   }
 
   // For compatibility with the legacy renderer, in case it's used with Fabric
@@ -249,12 +253,12 @@ export function prepareUpdate(
   newProps: Props,
   hostContext: HostContext,
 ): null | Object {
-  const viewConfig = instance.viewConfig;
+  const viewConfig = instance.canonical.viewConfig;
   const updatePayload = diff(oldProps, newProps, viewConfig.validAttributes);
   // TODO: If the event handlers have changed, we need to update the current props
   // in the commit phase but there is no host config hook to do it yet.
   // So instead we hack it by updating it in the render phase.
-  instance.currentProps = newProps;
+  instance.canonical.currentProps = newProps;
   return updatePayload;
 }
 
@@ -333,11 +337,7 @@ export function cloneInstance(
   }
   return {
     node: clone,
-    nativeTag: instance.nativeTag,
-    viewConfig: instance.viewConfig,
-    currentProps: instance.currentProps,
-    internalInstanceHandle: instance.internalInstanceHandle,
-    publicInstance: instance.publicInstance,
+    canonical: instance.canonical,
   };
 }
 
@@ -347,19 +347,15 @@ export function cloneHiddenInstance(
   props: Props,
   internalInstanceHandle: Object,
 ): Instance {
-  const viewConfig = instance.viewConfig;
+  const viewConfig = instance.canonical.viewConfig;
   const node = instance.node;
   const updatePayload = create(
     {style: {display: 'none'}},
     viewConfig.validAttributes,
   );
   return {
     node: cloneNodeWithNewProps(node, updatePayload),
-    nativeTag: instance.nativeTag,
-    viewConfig: instance.viewConfig,
-    currentProps: instance.currentProps,
-    internalInstanceHandle: instance.internalInstanceHandle,
-    publicInstance: instance.publicInstance,
+    canonical: instance.canonical,
   };
 }
 
diff --git a/packages/react-native-renderer/src/ReactNativeComponentTree.js b/packages/react-native-renderer/src/ReactNativeComponentTree.js
@@ -24,10 +24,10 @@ function getInstanceFromTag(tag) {
 function getTagFromInstance(inst) {
   let nativeInstance = inst.stateNode;
   let tag = nativeInstance._nativeTag;
-  if (tag === undefined) {
+  if (tag === undefined && nativeInstance.canonical != null) {
     // For compatibility with Fabric
-    tag = nativeInstance.nativeTag;
-    nativeInstance = nativeInstance.publicInstance;
+    tag = nativeInstance.canonical.nativeTag;
+    nativeInstance = nativeInstance.canonical.publicInstance;
   }
 
   if (!tag) {
diff --git a/packages/react-native-renderer/src/ReactNativeFiberInspector.js b/packages/react-native-renderer/src/ReactNativeFiberInspector.js
@@ -223,10 +223,11 @@ function getInspectorDataForViewAtPoint(
           }
 
           closestInstance =
-            internalInstanceHandle.stateNode.internalInstanceHandle;
+            internalInstanceHandle.stateNode.canonical.internalInstanceHandle;
 
           // Note: this is deprecated and we want to remove it ASAP. Keeping it here for React DevTools compatibility for now.
-          const nativeViewTag = internalInstanceHandle.stateNode.nativeTag;
+          const nativeViewTag =
+            internalInstanceHandle.stateNode.canonical.nativeTag;
 
           nativeFabricUIManager.measure(
             node,
diff --git a/packages/react-native-renderer/src/ReactNativeHostConfig.js b/packages/react-native-renderer/src/ReactNativeHostConfig.js
@@ -218,8 +218,8 @@ export function getChildHostContext(
 
 export function getPublicInstance(instance: Instance): * {
   // $FlowExpectedError[prop-missing] For compatibility with Fabric
-  if (instance.publicInstance != null) {
-    return instance.publicInstance;
+  if (instance.canonical != null && instance.canonical.publicInstance != null) {
+    return instance.canonical.publicInstance;
   }
 
   return instance;
diff --git a/packages/react-native-renderer/src/ReactNativePublicCompat.js b/packages/react-native-renderer/src/ReactNativePublicCompat.js
@@ -56,9 +56,12 @@ export function findHostInstance_DEPRECATED<TElementType: ElementType>(
   }
 
   // For compatibility with Fabric instances
-  if (componentOrHandle.publicInstance) {
+  if (
+    componentOrHandle.canonical &&
+    componentOrHandle.canonical.publicInstance
+  ) {
     // $FlowExpectedError[incompatible-return] Can't refine componentOrHandle as a Fabric instance
-    return componentOrHandle.publicInstance;
+    return componentOrHandle.canonical.publicInstance;
   }
 
   // For compatibility with legacy renderer instances
@@ -117,8 +120,11 @@ export function findNodeHandle(componentOrHandle: any): ?number {
   }
 
   // For compatibility with Fabric instances
-  if (componentOrHandle.nativeTag != null) {
-    return componentOrHandle.nativeTag;
+  if (
+    componentOrHandle.canonical != null &&
+    componentOrHandle.canonical.nativeTag != null
+  ) {
+    return componentOrHandle.canonical.nativeTag;
   }
 
   // For compatibility with Fabric public instances
