Skip to content

Commit

Permalink
Fixed incorrect value returned as public instance from reconciler (#2…
Browse files Browse the repository at this point in the history
…6283)

## Summary

A few methods in `ReactFiberReconciler` are supposed to return
`PublicInstance` values, but they return the `stateNode` from the fiber
directly. This assumes that the `stateNode` always matches the public
instance (which it does on Web) but that's not the case in React Native,
where the public instance is a field in that object.

This hasn't caused issues because everywhere where we use that method in
React Native we actually extract the real public instance from this
"fake" public instance.

This PR fixes the inconsistency and cleans up some code.

## How did you test this change?

Existing tests.
  • Loading branch information
rubennorte authored Mar 3, 2023
1 parent 25a8b97 commit b72ed69
Show file tree
Hide file tree
Showing 7 changed files with 31 additions and 51 deletions.
16 changes: 1 addition & 15 deletions packages/react-native-renderer/src/ReactFabric.js
Original file line number Diff line number Diff line change
Expand Up @@ -90,15 +90,6 @@ function findHostInstance_DEPRECATED<TElementType: ElementType>(
hostInstance = findHostInstance(componentOrHandle);
}

if (hostInstance == null) {
return hostInstance;
}
if ((hostInstance: any).canonical) {
// Fabric
return (hostInstance: any).canonical;
}
// $FlowFixMe[incompatible-return]
// $FlowFixMe[incompatible-exact]
return hostInstance;
}

Expand Down Expand Up @@ -146,12 +137,7 @@ function findNodeHandle(componentOrHandle: any): ?number {
if (hostInstance == null) {
return hostInstance;
}
// TODO: the code is right but the types here are wrong.
// https://github.com/facebook/react/pull/12863
if ((hostInstance: any).canonical) {
// Fabric
return (hostInstance: any).canonical._nativeTag;
}

return hostInstance._nativeTag;
}

Expand Down
19 changes: 12 additions & 7 deletions packages/react-native-renderer/src/ReactFabricHostConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ if (registerEventHandler) {
/**
* This is used for refs on host components.
*/
class ReactFabricHostComponent {
class ReactFabricHostComponent implements NativeMethods {
_nativeTag: number;
viewConfig: ViewConfig;
currentProps: Props;
Expand Down Expand Up @@ -215,10 +215,6 @@ class ReactFabricHostComponent {
}
}

// $FlowFixMe[class-object-subtyping] found when upgrading Flow
// $FlowFixMe[method-unbinding] found when upgrading Flow
(ReactFabricHostComponent.prototype: $ReadOnly<{...NativeMethods, ...}>);

export * from 'react-reconciler/src/ReactFiberHostConfigWithNoMutation';
export * from 'react-reconciler/src/ReactFiberHostConfigWithNoHydration';
export * from 'react-reconciler/src/ReactFiberHostConfigWithNoScopes';
Expand Down Expand Up @@ -342,8 +338,17 @@ export function getChildHostContext(
}
}

export function getPublicInstance(instance: Instance): * {
return instance.canonical;
export function getPublicInstance(instance: Instance): null | PublicInstance {
if (instance.canonical) {
return instance.canonical;
}

// For compatibility with Paper
if (instance._nativeTag != null) {
return instance;
}

return null;
}

export function prepareForCommit(containerInfo: Container): null | Object {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ import {
warnForStyleProps,
} from './NativeMethodsMixinUtils';

class ReactNativeFiberHostComponent {
class ReactNativeFiberHostComponent implements NativeMethods {
_children: Array<Instance | number>;
_nativeTag: number;
_internalFiberInstanceHandleDEV: Object;
Expand Down Expand Up @@ -127,8 +127,4 @@ class ReactNativeFiberHostComponent {
}
}

// $FlowFixMe[class-object-subtyping] found when upgrading Flow
// $FlowFixMe[method-unbinding] found when upgrading Flow
(ReactNativeFiberHostComponent.prototype: $ReadOnly<{...NativeMethods, ...}>);

export default ReactNativeFiberHostComponent;
5 changes: 5 additions & 0 deletions packages/react-native-renderer/src/ReactNativeHostConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,11 @@ export function getChildHostContext(
}

export function getPublicInstance(instance: Instance): * {
// $FlowExpectedError[prop-missing] For compatibility with Fabric
if (instance.canonical) {
return instance.canonical;
}

return instance;
}

Expand Down
14 changes: 1 addition & 13 deletions packages/react-native-renderer/src/ReactNativeRenderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -89,15 +89,6 @@ function findHostInstance_DEPRECATED(
hostInstance = findHostInstance(componentOrHandle);
}

if (hostInstance == null) {
return hostInstance;
}
if ((hostInstance: any).canonical) {
// Fabric
return (hostInstance: any).canonical;
}
// $FlowFixMe[incompatible-return]
// $FlowFixMe[incompatible-exact]
return hostInstance;
}

Expand Down Expand Up @@ -145,10 +136,7 @@ function findNodeHandle(componentOrHandle: any): ?number {
if (hostInstance == null) {
return hostInstance;
}
if ((hostInstance: any).canonical) {
// Fabric
return (hostInstance: any).canonical._nativeTag;
}

return hostInstance._nativeTag;
}

Expand Down
16 changes: 8 additions & 8 deletions packages/react-native-renderer/src/ReactNativeTypes.js
Original file line number Diff line number Diff line change
Expand Up @@ -95,18 +95,18 @@ export type PartialViewConfig = $ReadOnly<{
validAttributes?: PartialAttributeConfiguration,
}>;

export type NativeMethods = $ReadOnly<{
blur(): void,
focus(): void,
measure(callback: MeasureOnSuccessCallback): void,
measureInWindow(callback: MeasureInWindowOnSuccessCallback): void,
export interface NativeMethods {
blur(): void;
focus(): void;
measure(callback: MeasureOnSuccessCallback): void;
measureInWindow(callback: MeasureInWindowOnSuccessCallback): void;
measureLayout(
relativeToNativeNode: number | ElementRef<HostComponent<mixed>>,
onSuccess: MeasureLayoutOnSuccessCallback,
onFail?: () => void,
): void,
setNativeProps(nativeProps: {...}): void,
}>;
): void;
setNativeProps(nativeProps: {...}): void;
}

export type HostComponent<T> = AbstractComponent<T, $ReadOnly<NativeMethods>>;

Expand Down
6 changes: 3 additions & 3 deletions packages/react-reconciler/src/ReactFiberReconciler.js
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ function findHostInstance(component: Object): PublicInstance | null {
if (hostFiber === null) {
return null;
}
return hostFiber.stateNode;
return getPublicInstance(hostFiber.stateNode);
}

function findHostInstanceWithWarning(
Expand Down Expand Up @@ -240,7 +240,7 @@ function findHostInstanceWithWarning(
}
}
}
return hostFiber.stateNode;
return getPublicInstance(hostFiber.stateNode);
}
return findHostInstance(component);
}
Expand Down Expand Up @@ -524,7 +524,7 @@ export function findHostInstanceWithNoPortals(
if (hostFiber === null) {
return null;
}
return hostFiber.stateNode;
return getPublicInstance(hostFiber.stateNode);
}

let shouldErrorImpl: Fiber => ?boolean = fiber => null;
Expand Down

0 comments on commit b72ed69

Please sign in to comment.