From 108aed083e6f53c14d2b87cbd960fe3fb53fdb37 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20Norte?= Date: Fri, 17 Mar 2023 11:27:49 +0000 Subject: [PATCH] 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. --- .../src/ReactFabricComponentTree.js | 9 ++-- .../src/ReactFabricHostConfig.js | 50 +++++++++---------- .../src/ReactNativeComponentTree.js | 6 +-- .../src/ReactNativeFiberInspector.js | 5 +- .../src/ReactNativeHostConfig.js | 4 +- .../src/ReactNativePublicCompat.js | 14 ++++-- 6 files changed, 47 insertions(+), 41 deletions(-) diff --git a/packages/react-native-renderer/src/ReactFabricComponentTree.js b/packages/react-native-renderer/src/ReactFabricComponentTree.js index b1cc8642de..c295c42ed1 100644 --- 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 index 0b2dfd0ffe..689dabcfeb 100644 --- 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,7 +347,7 @@ 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'}}, @@ -355,11 +355,7 @@ export function cloneHiddenInstance( ); 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 index 7021610daf..bd3f1f4414 100644 --- 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 index 57d9b0542c..94f9abf35f 100644 --- 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 index 330875deba..501d343b19 100644 --- 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 index 527983af6d..a2979666d7 100644 --- a/packages/react-native-renderer/src/ReactNativePublicCompat.js +++ b/packages/react-native-renderer/src/ReactNativePublicCompat.js @@ -56,9 +56,12 @@ export function findHostInstance_DEPRECATED( } // 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