From 0700dd50bda98f5ee86f2e3adfe5e9906ed1e8e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20Norte?= Date: Tue, 4 Apr 2023 15:43:35 +0200 Subject: [PATCH] Implement public instances for text nodes in Fabric (#26516) ## 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 https://github.com/react-native-community/discussions-and-proposals/pull/607 ## How did you test this change? Added unit tests (also fixed a test that was only testing the logic in a mock :S). --- .../src/ReactFabricHostConfig.js | 52 ++++++++-- .../src/ReactNativeTypes.js | 3 +- .../ReactNativePrivateInterface.js | 4 + .../ReactPrivate/createPublicTextInstance.js | 18 ++++ .../__tests__/ReactFabric-test.internal.js | 95 ++++++++++++++++--- scripts/flow/react-native-host-hooks.js | 4 + 6 files changed, 151 insertions(+), 25 deletions(-) create mode 100644 packages/react-native-renderer/src/__mocks__/react-native/Libraries/ReactPrivate/createPublicTextInstance.js diff --git a/packages/react-native-renderer/src/ReactFabricHostConfig.js b/packages/react-native-renderer/src/ReactFabricHostConfig.js index 5494f4152a47e..72109ca7c3ec1 100644 --- a/packages/react-native-renderer/src/ReactFabricHostConfig.js +++ b/packages/react-native-renderer/src/ReactFabricHostConfig.js @@ -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 { @@ -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; @@ -115,7 +128,7 @@ export function createInstance( props: Props, rootContainerInstance: Container, hostContext: HostContext, - internalInstanceHandle: Object, + internalInstanceHandle: InternalInstanceHandle, ): Instance { const tag = nextReactTag; nextReactTag += 2; @@ -162,7 +175,7 @@ export function createTextInstance( text: string, rootContainerInstance: Container, hostContext: HostContext, - internalInstanceHandle: Object, + internalInstanceHandle: InternalInstanceHandle, ): TextInstance { if (__DEV__) { if (!hostContext.isInAParentText) { @@ -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); } @@ -321,7 +351,7 @@ export function cloneInstance( type: string, oldProps: Props, newProps: Props, - internalInstanceHandle: Object, + internalInstanceHandle: InternalInstanceHandle, keepChildren: boolean, recyclableInstance: null | Instance, ): Instance { @@ -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; @@ -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.'); } @@ -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 } diff --git a/packages/react-native-renderer/src/ReactNativeTypes.js b/packages/react-native-renderer/src/ReactNativeTypes.js index 129613d54e7cf..a6063cb2c10a0 100644 --- a/packages/react-native-renderer/src/ReactNativeTypes.js +++ b/packages/react-native-renderer/src/ReactNativeTypes.js @@ -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( @@ -244,7 +245,7 @@ export type ReactFabricType = { ): ?Node, getPublicInstanceFromInternalInstanceHandle( internalInstanceHandle: InternalInstanceHandle, - ): PublicInstance, + ): PublicInstance | PublicTextInstance, ... }; diff --git a/packages/react-native-renderer/src/__mocks__/react-native/Libraries/ReactPrivate/ReactNativePrivateInterface.js b/packages/react-native-renderer/src/__mocks__/react-native/Libraries/ReactPrivate/ReactNativePrivateInterface.js index 03b89b3e711f3..d1234fe8a876a 100644 --- a/packages/react-native-renderer/src/__mocks__/react-native/Libraries/ReactPrivate/ReactNativePrivateInterface.js +++ b/packages/react-native-renderer/src/__mocks__/react-native/Libraries/ReactPrivate/ReactNativePrivateInterface.js @@ -8,6 +8,7 @@ */ export opaque type PublicInstance = mixed; +export opaque type PublicTextInstance = mixed; module.exports = { get BatchedBridge() { @@ -55,4 +56,7 @@ module.exports = { get createPublicInstance() { return require('./createPublicInstance').default; }, + get createPublicTextInstance() { + return require('./createPublicTextInstance').default; + }, }; diff --git a/packages/react-native-renderer/src/__mocks__/react-native/Libraries/ReactPrivate/createPublicTextInstance.js b/packages/react-native-renderer/src/__mocks__/react-native/Libraries/ReactPrivate/createPublicTextInstance.js new file mode 100644 index 0000000000000..a4527bff9f69a --- /dev/null +++ b/packages/react-native-renderer/src/__mocks__/react-native/Libraries/ReactPrivate/createPublicTextInstance.js @@ -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, + }; +} diff --git a/packages/react-native-renderer/src/__tests__/ReactFabric-test.internal.js b/packages/react-native-renderer/src/__tests__/ReactFabric-test.internal.js index cdc8882a08554..4e97cead6f349 100644 --- a/packages/react-native-renderer/src/__tests__/ReactFabric-test.internal.js +++ b/packages/react-native-renderer/src/__tests__/ReactFabric-test.internal.js @@ -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 " + @@ -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; }); @@ -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 () => { @@ -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 () => { @@ -1015,6 +1014,30 @@ describe('ReactFabric', () => { uiViewClassName: 'RCTView', })); + await act(() => { + ReactFabric.render(, 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( @@ -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(Text content, 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); }); }); diff --git a/scripts/flow/react-native-host-hooks.js b/scripts/flow/react-native-host-hooks.js index d3f5d3b7b50c5..f7a5f26e6c2ea 100644 --- a/scripts/flow/react-native-host-hooks.js +++ b/scripts/flow/react-native-host-hooks.js @@ -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; @@ -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' {