Skip to content

Commit

Permalink
Fix use of stale props in Fabric events (#26408)
Browse files Browse the repository at this point in the history
## 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.
  • Loading branch information
rubennorte committed Mar 17, 2023
1 parent 8fa41ff commit 108aed0
Show file tree
Hide file tree
Showing 6 changed files with 47 additions and 41 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -41,7 +44,7 @@ function getNodeFromInstance(fiber: Fiber): PublicInstance {
}

function getFiberCurrentPropsFromNode(instance: Instance): Props {
return instance.currentProps;
return instance.canonical.currentProps;
}

export {
Expand Down
50 changes: 23 additions & 27 deletions packages/react-native-renderer/src/ReactFabricHostConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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,
},
};
}

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

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

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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
4 changes: 2 additions & 2 deletions packages/react-native-renderer/src/ReactNativeHostConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
14 changes: 10 additions & 4 deletions packages/react-native-renderer/src/ReactNativePublicCompat.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit 108aed0

Please sign in to comment.