Skip to content

Commit

Permalink
Implement public instances for text nodes in Fabric (#26516)
Browse files Browse the repository at this point in the history
## Summary

This adds the ability to create public instances for text nodes in
Fabric. The implementation for the public instances lives in React
Native (as it does for host components after #26437). The logic here
just handles their lazy instantiation when requested via
`getPublicInstanceFromInternalInstanceHandle`, which is called by Fabric
with information coming from the shadow tree.

It's important that the creation of public instances for text nodes is
done lazily to avoid regressing memory usage when unused. Instances for
text nodes are left intact if the public instance is never accessed.

This is necessary to implement access to text nodes in React Native as
explained in
react-native-community/discussions-and-proposals#607

## How did you test this change?

Added unit tests (also fixed a test that was only testing the logic in a
mock :S).
  • Loading branch information
rubennorte authored Apr 4, 2023
1 parent 4a1cc2d commit 0700dd5
Show file tree
Hide file tree
Showing 6 changed files with 151 additions and 25 deletions.
52 changes: 42 additions & 10 deletions packages/react-native-renderer/src/ReactFabricHostConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,16 @@ import {
DefaultEventPriority,
DiscreteEventPriority,
} from 'react-reconciler/src/ReactEventPriorities';
import {HostText} from 'react-reconciler/src/ReactWorkTags';

// Modules provided by RN:
import {
ReactNativeViewConfigRegistry,
deepFreezeAndThrowOnMutationInDev,
createPublicInstance,
createPublicTextInstance,
type PublicInstance as ReactNativePublicInstance,
type PublicTextInstance,
} from 'react-native/Libraries/ReactPrivate/ReactNativePrivateInterface';

const {
Expand All @@ -47,23 +50,33 @@ const {get: getViewConfigForType} = ReactNativeViewConfigRegistry;
// This means that they never overlap.
let nextReactTag = 2;

type InternalInstanceHandle = Object;
type Node = Object;
export type Type = string;
export type Props = Object;
export type Instance = {
// Reference to the shadow node.
node: Node,
// This object is shared by all the clones of the instance.
// We use it to access their shared public instance (exposed through refs)
// and to access its committed state for events, etc.
canonical: {
nativeTag: number,
viewConfig: ViewConfig,
currentProps: Props,
// Reference to the React handle (the fiber)
internalInstanceHandle: Object,
internalInstanceHandle: InternalInstanceHandle,
// Exposed through refs.
publicInstance: PublicInstance,
},
};
export type TextInstance = {node: Node, ...};
export type TextInstance = {
// Reference to the shadow node.
node: Node,
// Text instances are never cloned, so we don't need to keep a "canonical"
// reference to make sure all clones of the instance point to the same values.
publicInstance?: PublicTextInstance,
};
export type HydratableInstance = Instance | TextInstance;
export type PublicInstance = ReactNativePublicInstance;
export type Container = number;
Expand Down Expand Up @@ -115,7 +128,7 @@ export function createInstance(
props: Props,
rootContainerInstance: Container,
hostContext: HostContext,
internalInstanceHandle: Object,
internalInstanceHandle: InternalInstanceHandle,
): Instance {
const tag = nextReactTag;
nextReactTag += 2;
Expand Down Expand Up @@ -162,7 +175,7 @@ export function createTextInstance(
text: string,
rootContainerInstance: Container,
hostContext: HostContext,
internalInstanceHandle: Object,
internalInstanceHandle: InternalInstanceHandle,
): TextInstance {
if (__DEV__) {
if (!hostContext.isInAParentText) {
Expand Down Expand Up @@ -239,9 +252,26 @@ export function getPublicInstance(instance: Instance): null | PublicInstance {
return null;
}

function getPublicTextInstance(
textInstance: TextInstance,
internalInstanceHandle: InternalInstanceHandle,
): PublicTextInstance {
if (textInstance.publicInstance == null) {
textInstance.publicInstance = createPublicTextInstance(
internalInstanceHandle,
);
}
return textInstance.publicInstance;
}

export function getPublicInstanceFromInternalInstanceHandle(
internalInstanceHandle: Object,
): null | PublicInstance {
internalInstanceHandle: InternalInstanceHandle,
): null | PublicInstance | PublicTextInstance {
if (internalInstanceHandle.tag === HostText) {
const textInstance: TextInstance = internalInstanceHandle.stateNode;
return getPublicTextInstance(textInstance, internalInstanceHandle);
}

const instance: Instance = internalInstanceHandle.stateNode;
return getPublicInstance(instance);
}
Expand Down Expand Up @@ -321,7 +351,7 @@ export function cloneInstance(
type: string,
oldProps: Props,
newProps: Props,
internalInstanceHandle: Object,
internalInstanceHandle: InternalInstanceHandle,
keepChildren: boolean,
recyclableInstance: null | Instance,
): Instance {
Expand Down Expand Up @@ -350,7 +380,7 @@ export function cloneHiddenInstance(
instance: Instance,
type: string,
props: Props,
internalInstanceHandle: Object,
internalInstanceHandle: InternalInstanceHandle,
): Instance {
const viewConfig = instance.canonical.viewConfig;
const node = instance.node;
Expand All @@ -367,7 +397,7 @@ export function cloneHiddenInstance(
export function cloneHiddenTextInstance(
instance: Instance,
text: string,
internalInstanceHandle: Object,
internalInstanceHandle: InternalInstanceHandle,
): TextInstance {
throw new Error('Not yet implemented.');
}
Expand Down Expand Up @@ -399,7 +429,9 @@ export function getInstanceFromNode(node: any): empty {
throw new Error('Not yet implemented.');
}

export function beforeActiveInstanceBlur(internalInstanceHandle: Object) {
export function beforeActiveInstanceBlur(
internalInstanceHandle: InternalInstanceHandle,
) {
// noop
}

Expand Down
3 changes: 2 additions & 1 deletion packages/react-native-renderer/src/ReactNativeTypes.js
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,7 @@ export type ReactNativeType = {
export opaque type Node = mixed;
export opaque type InternalInstanceHandle = mixed;
type PublicInstance = mixed;
type PublicTextInstance = mixed;

export type ReactFabricType = {
findHostInstance_DEPRECATED<TElementType: ElementType>(
Expand Down Expand Up @@ -244,7 +245,7 @@ export type ReactFabricType = {
): ?Node,
getPublicInstanceFromInternalInstanceHandle(
internalInstanceHandle: InternalInstanceHandle,
): PublicInstance,
): PublicInstance | PublicTextInstance,
...
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
*/

export opaque type PublicInstance = mixed;
export opaque type PublicTextInstance = mixed;

module.exports = {
get BatchedBridge() {
Expand Down Expand Up @@ -55,4 +56,7 @@ module.exports = {
get createPublicInstance() {
return require('./createPublicInstance').default;
},
get createPublicTextInstance() {
return require('./createPublicTextInstance').default;
},
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/**
* Copyright (c) Meta Platforms, Inc. and affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @flow strict
*/

import type {PublicInstance} from './ReactNativePrivateInterface';

export default function createPublicTextInstance(
internalInstanceHandle: mixed,
): PublicInstance {
return {
__internalInstanceHandle: internalInstanceHandle,
};
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,10 @@

let React;
let ReactFabric;
let ReactNativePrivateInterface;
let createReactNativeComponentClass;
let StrictMode;
let act;
let getNativeTagFromPublicInstance;
let getNodeFromPublicInstance;

const DISPATCH_COMMAND_REQUIRES_HOST_COMPONENT =
"Warning: dispatchCommand was called with a ref that isn't a " +
Expand All @@ -39,14 +38,10 @@ describe('ReactFabric', () => {
React = require('react');
StrictMode = React.StrictMode;
ReactFabric = require('react-native-renderer/fabric');
ReactNativePrivateInterface = require('react-native/Libraries/ReactPrivate/ReactNativePrivateInterface');
createReactNativeComponentClass =
require('react-native/Libraries/ReactPrivate/ReactNativePrivateInterface')
.ReactNativeViewConfigRegistry.register;
getNativeTagFromPublicInstance =
require('react-native/Libraries/ReactPrivate/ReactNativePrivateInterface').getNativeTagFromPublicInstance;
getNodeFromPublicInstance =
require('react-native/Libraries/ReactPrivate/ReactNativePrivateInterface').getNodeFromPublicInstance;

act = require('internal-test-utils').act;
});

Expand Down Expand Up @@ -937,7 +932,9 @@ describe('ReactFabric', () => {
'\n in RCTView (at **)' +
'\n in ContainsStrictModeChild (at **)',
]);
expect(match).toBe(getNativeTagFromPublicInstance(child));
expect(match).toBe(
ReactNativePrivateInterface.getNativeTagFromPublicInstance(child),
);
});

it('findNodeHandle should warn if passed a component that is inside StrictMode', async () => {
Expand Down Expand Up @@ -974,7 +971,9 @@ describe('ReactFabric', () => {
'\n in RCTView (at **)' +
'\n in IsInStrictMode (at **)',
]);
expect(match).toBe(getNativeTagFromPublicInstance(child));
expect(match).toBe(
ReactNativePrivateInterface.getNativeTagFromPublicInstance(child),
);
});

it('should no-op if calling sendAccessibilityEvent on unmounted refs', async () => {
Expand Down Expand Up @@ -1015,6 +1014,30 @@ describe('ReactFabric', () => {
uiViewClassName: 'RCTView',
}));

await act(() => {
ReactFabric.render(<View foo="test" />, 1);
});

const internalInstanceHandle =
nativeFabricUIManager.createNode.mock.calls[0][4];
expect(internalInstanceHandle).toEqual(expect.any(Object));

const expectedShadowNode =
nativeFabricUIManager.createNode.mock.results[0].value;
expect(expectedShadowNode).toEqual(expect.any(Object));

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

it('getPublicInstanceFromInternalInstanceHandle should provide public instances for HostComponent', async () => {
const View = createReactNativeComponentClass('RCTView', () => ({
validAttributes: {foo: true},
uiViewClassName: 'RCTView',
}));

let viewRef;
await act(() => {
ReactFabric.render(
Expand All @@ -1028,11 +1051,55 @@ describe('ReactFabric', () => {
);
});

const expectedShadowNode =
nativeFabricUIManager.createNode.mock.results[0].value;
expect(expectedShadowNode).toEqual(expect.any(Object));
const internalInstanceHandle =
nativeFabricUIManager.createNode.mock.calls[0][4];
expect(internalInstanceHandle).toEqual(expect.any(Object));

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

it('getPublicInstanceFromInternalInstanceHandle should provide public instances for HostText', async () => {
jest.spyOn(ReactNativePrivateInterface, 'createPublicTextInstance');

const RCTText = createReactNativeComponentClass('RCTText', () => ({
validAttributes: {},
uiViewClassName: 'RCTText',
}));

await act(() => {
ReactFabric.render(<RCTText>Text content</RCTText>, 1);
});

// Access the internal instance handle used to create the text node.
const internalInstanceHandle =
nativeFabricUIManager.createNode.mock.calls[0][4];
expect(internalInstanceHandle).toEqual(expect.any(Object));

// Text public instances should be created lazily.
expect(
ReactNativePrivateInterface.createPublicTextInstance,
).not.toHaveBeenCalled();

const publicInstance =
ReactFabric.getPublicInstanceFromInternalInstanceHandle(
internalInstanceHandle,
);

// We just requested the text public instance, so it should have been created at this point.
expect(
ReactNativePrivateInterface.createPublicTextInstance,
).toHaveBeenCalledTimes(1);
expect(
ReactNativePrivateInterface.createPublicTextInstance,
).toHaveBeenCalledWith(internalInstanceHandle);

const expectedPublicInstance =
ReactNativePrivateInterface.createPublicTextInstance.mock.results[0]
.value;
expect(publicInstance).toBe(expectedPublicInstance);
});
});
4 changes: 4 additions & 0 deletions scripts/flow/react-native-host-hooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,7 @@ declare module 'react-native/Libraries/ReactPrivate/ReactNativePrivateInterface'
...
};
declare export opaque type PublicInstance;
declare export opaque type PublicTextInstance;
declare export function getNodeFromPublicInstance(
publicInstance: PublicInstance,
): Object;
Expand All @@ -156,6 +157,9 @@ declare module 'react-native/Libraries/ReactPrivate/ReactNativePrivateInterface'
viewConfig: __ViewConfig,
internalInstanceHandle: mixed,
): PublicInstance;
declare export function createPublicTextInstance(
internalInstanceHandle: mixed,
): PublicTextInstance;
}

declare module 'react-native/Libraries/ReactPrivate/ReactNativePrivateInitializeCore' {
Expand Down

0 comments on commit 0700dd5

Please sign in to comment.