Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clean interface for public instances between React and React Native #26416

Merged
merged 2 commits into from
Mar 20, 2023
Merged

Clean interface for public instances between React and React Native #26416

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c5b4b2b
Provide function to access public instance from internal instance han…
rubennorte Mar 17, 2023
b78f0b3
Replace getInstanceHandleFromPublicInstance with getNodeFromPublicIns…
rubennorte Mar 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions packages/react-native-renderer/src/ReactFabric.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ import {
sendAccessibilityEvent,
getNodeFromInternalInstanceHandle,
} from './ReactNativePublicCompat';
import {getPublicInstanceFromInternalInstanceHandle} from './ReactFabricHostConfig';

// $FlowFixMe[missing-local-annot]
function onRecoverableError(error) {
Expand Down Expand Up @@ -124,6 +125,10 @@ export {
// This method allows it to acess the most recent shadow node for
// the instance (it's only accessible through it).
getNodeFromInternalInstanceHandle,
// Fabric native methods to traverse the host tree return the same internal
// instance handles we use to dispatch events. This provides a way to access
// the public instances we created from them (potentially created lazily).
getPublicInstanceFromInternalInstanceHandle,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the difference between a Node and a PublicInstance (i.e. why doesn't the existing getNodeFromInternalInstanceHandle work)?

Copy link
Contributor Author

@rubennorte rubennorte Mar 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Node is the shadow node (which is a host object created by Fabric in native). Public instance is the instance of ReactFabricHostComponent that's provided via refs and has the methods that users can invoke directly (like measure and focus, but will evolve to have things like children, getBoundingClientRect, etc. as per my proposal).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if these are supposed to be different things? I think I would expect a Node to be a HostComponent but I don't have a ton of context.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They're the same concept on Web but not on RN. If we made them to be the same we'd have to implement the methods of the public instance in native, which isn't very convenient.

We could use another refactor to clean up naming, improve docs, etc. I can give it a try after these 2 PRs.

};

injectIntoDevTools({
Expand Down
7 changes: 7 additions & 0 deletions packages/react-native-renderer/src/ReactFabricHostConfig.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,13 @@ export function getPublicInstance(instance: Instance): null | PublicInstance {
return null;
}

export function getPublicInstanceFromInternalInstanceHandle(
internalInstanceHandle: Object,
): null | PublicInstance {
const instance: Instance = internalInstanceHandle.stateNode;
return getPublicInstance(instance);
}

export function prepareForCommit(containerInfo: Container): null | Object {
// Noop
return null;
Expand Down
11 changes: 9 additions & 2 deletions packages/react-native-renderer/src/ReactFabricPublicInstanceUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
*/

import type {ReactFabricHostComponent} from './ReactFabricPublicInstance';
import {getNodeFromInternalInstanceHandle} from './ReactNativePublicCompat';

/**
* IMPORTANT: This module is used in Paper and Fabric. It needs to be defined
Expand All @@ -22,8 +23,14 @@ export function getNativeTagFromPublicInstance(
return publicInstance.__nativeTag;
}

export function getInternalInstanceHandleFromPublicInstance(
export function getNodeFromPublicInstance(
publicInstance: ReactFabricHostComponent,
): mixed {
return publicInstance.__internalInstanceHandle;
if (publicInstance.__internalInstanceHandle == null) {
return null;
}

return getNodeFromInternalInstanceHandle(
publicInstance.__internalInstanceHandle,
);
}
9 changes: 2 additions & 7 deletions packages/react-native-renderer/src/ReactNativeFiberInspector.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import {HostComponent} from 'react-reconciler/src/ReactWorkTags';
import {UIManager} from 'react-native/Libraries/ReactPrivate/ReactNativePrivateInterface';
import {enableGetInspectorDataForInstanceInProduction} from 'shared/ReactFeatureFlags';
import {getClosestInstanceFromNode} from './ReactNativeComponentTree';
import {getInternalInstanceHandleFromPublicInstance} from './ReactFabricPublicInstanceUtils';
import {getNodeFromPublicInstance} from './ReactFabricPublicInstanceUtils';
import {getNodeFromInternalInstanceHandle} from './ReactNativePublicCompat';

const emptyObject = {};
Expand Down Expand Up @@ -196,12 +196,7 @@ function getInspectorDataForViewAtPoint(
if (__DEV__) {
let closestInstance = null;

const fabricInstanceHandle =
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This level of indirection was unnecessary.

getInternalInstanceHandleFromPublicInstance(inspectedView);
const fabricNode =
fabricInstanceHandle != null
? getNodeFromInternalInstanceHandle(fabricInstanceHandle)
: null;
const fabricNode = getNodeFromPublicInstance(inspectedView);
if (fabricNode) {
// For Fabric we can look up the instance handle directly and measure it.
nativeFabricUIManager.findNodeAtPoint(
Expand Down
22 changes: 7 additions & 15 deletions packages/react-native-renderer/src/ReactNativePublicCompat.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import ReactSharedInternals from 'shared/ReactSharedInternals';
import getComponentNameFromType from 'shared/getComponentNameFromType';

import {
getInternalInstanceHandleFromPublicInstance,
getNodeFromPublicInstance,
getNativeTagFromPublicInstance,
} from './ReactFabricPublicInstanceUtils';

Expand Down Expand Up @@ -176,14 +176,10 @@ export function dispatchCommand(
return;
}

const internalInstanceHandle =
getInternalInstanceHandleFromPublicInstance(handle);
const node = getNodeFromPublicInstance(handle);

if (internalInstanceHandle != null) {
const node = getNodeFromInternalInstanceHandle(internalInstanceHandle);
if (node != null) {
nativeFabricUIManager.dispatchCommand(node, command, args);
}
if (node != null) {
nativeFabricUIManager.dispatchCommand(node, command, args);
} else {
UIManager.dispatchViewManagerCommand(nativeTag, command, args);
}
Expand All @@ -204,13 +200,9 @@ export function sendAccessibilityEvent(handle: any, eventType: string) {
return;
}

const internalInstanceHandle =
getInternalInstanceHandleFromPublicInstance(handle);
if (internalInstanceHandle != null) {
const node = getNodeFromInternalInstanceHandle(internalInstanceHandle);
if (node != null) {
nativeFabricUIManager.sendAccessibilityEvent(node, eventType);
}
const node = getNodeFromPublicInstance(handle);
if (node != null) {
nativeFabricUIManager.sendAccessibilityEvent(node, eventType);
} else {
legacySendAccessibilityEvent(nativeTag, eventType);
}
Expand Down
9 changes: 8 additions & 1 deletion packages/react-native-renderer/src/ReactNativeTypes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,8 @@ export type ReactNativeType = {
};

export opaque type Node = mixed;
type InternalInstanceHandle = mixed;
type PublicInstance = mixed;

export type ReactFabricType = {
findHostInstance_DEPRECATED<TElementType: ElementType>(
Expand All @@ -237,7 +239,12 @@ export type ReactFabricType = {
concurrentRoot: ?boolean,
): ?ElementRef<ElementType>,
unmountComponentAtNode(containerTag: number): void,
getNodeFromInternalInstanceHandle(internalInstanceHandle: mixed): ?Node,
getNodeFromInternalInstanceHandle(
internalInstanceHandle: InternalInstanceHandle,
): ?Node,
getPublicInstanceFromInternalInstanceHandle(
internalInstanceHandle: InternalInstanceHandle,
): PublicInstance,
...
};

Expand Down
13 changes: 5 additions & 8 deletions packages/react-native-renderer/src/__tests__/ReactFabric-test.internal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ let createReactNativeComponentClass;
let StrictMode;
let act;
let getNativeTagFromPublicInstance;
let getInternalInstanceHandleFromPublicInstance;
let getNodeFromPublicInstance;

const DISPATCH_COMMAND_REQUIRES_HOST_COMPONENT =
"Warning: dispatchCommand was called with a ref that isn't a " +
Expand Down Expand Up @@ -44,8 +44,8 @@ describe('ReactFabric', () => {
.ReactNativeViewConfigRegistry.register;
getNativeTagFromPublicInstance =
require('../ReactFabricPublicInstanceUtils').getNativeTagFromPublicInstance;
getInternalInstanceHandleFromPublicInstance =
require('../ReactFabricPublicInstanceUtils').getInternalInstanceHandleFromPublicInstance;
getNodeFromPublicInstance =
require('../ReactFabricPublicInstanceUtils').getNodeFromPublicInstance;

act = require('internal-test-utils').act;
});
Expand Down Expand Up @@ -1032,10 +1032,7 @@ describe('ReactFabric', () => {
nativeFabricUIManager.createNode.mock.results[0].value;
expect(expectedShadowNode).toEqual(expect.any(Object));

const internalInstanceHandle =
getInternalInstanceHandleFromPublicInstance(viewRef);
expect(
ReactFabric.getNodeFromInternalInstanceHandle(internalInstanceHandle),
).toBe(expectedShadowNode);
const node = getNodeFromPublicInstance(viewRef);
expect(node).toBe(expectedShadowNode);
});
});