From dd56e2a1c436fc714718bc588a5518e4d337eaab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20Norte?= Date: Thu, 30 Mar 2023 13:32:08 +0100 Subject: [PATCH 1/6] Define type alias for InternalInstanceHandle --- .../src/ReactFabricHostConfig.js | 22 ++++++++++++------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/packages/react-native-renderer/src/ReactFabricHostConfig.js b/packages/react-native-renderer/src/ReactFabricHostConfig.js index 5494f4152a47e..f8a78db5888f4 100644 --- a/packages/react-native-renderer/src/ReactFabricHostConfig.js +++ b/packages/react-native-renderer/src/ReactFabricHostConfig.js @@ -47,18 +47,22 @@ 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, }, @@ -115,7 +119,7 @@ export function createInstance( props: Props, rootContainerInstance: Container, hostContext: HostContext, - internalInstanceHandle: Object, + internalInstanceHandle: InternalInstanceHandle, ): Instance { const tag = nextReactTag; nextReactTag += 2; @@ -162,7 +166,7 @@ export function createTextInstance( text: string, rootContainerInstance: Container, hostContext: HostContext, - internalInstanceHandle: Object, + internalInstanceHandle: InternalInstanceHandle, ): TextInstance { if (__DEV__) { if (!hostContext.isInAParentText) { @@ -240,7 +244,7 @@ export function getPublicInstance(instance: Instance): null | PublicInstance { } export function getPublicInstanceFromInternalInstanceHandle( - internalInstanceHandle: Object, + internalInstanceHandle: InternalInstanceHandle, ): null | PublicInstance { const instance: Instance = internalInstanceHandle.stateNode; return getPublicInstance(instance); @@ -321,7 +325,7 @@ export function cloneInstance( type: string, oldProps: Props, newProps: Props, - internalInstanceHandle: Object, + internalInstanceHandle: InternalInstanceHandle, keepChildren: boolean, recyclableInstance: null | Instance, ): Instance { @@ -350,7 +354,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 +371,7 @@ export function cloneHiddenInstance( export function cloneHiddenTextInstance( instance: Instance, text: string, - internalInstanceHandle: Object, + internalInstanceHandle: InternalInstanceHandle, ): TextInstance { throw new Error('Not yet implemented.'); } @@ -399,7 +403,9 @@ export function getInstanceFromNode(node: any): empty { throw new Error('Not yet implemented.'); } -export function beforeActiveInstanceBlur(internalInstanceHandle: Object) { +export function beforeActiveInstanceBlur( + internalInstanceHandle: InternalInstanceHandle, +) { // noop } From c94437c0f5bf6e3f79a9dfaadbdc60b8fdcb4248 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20Norte?= Date: Thu, 30 Mar 2023 13:32:42 +0100 Subject: [PATCH 2/6] Create public instances for text nodes lazily --- .../src/ReactFabricHostConfig.js | 30 +++++++++++++++++-- .../ReactNativePrivateInterface.js | 4 +++ .../ReactPrivate/createPublicTextInstance.js | 18 +++++++++++ scripts/flow/react-native-host-hooks.js | 4 +++ 4 files changed, 54 insertions(+), 2 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 f8a78db5888f4..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 { @@ -67,7 +70,13 @@ export type Instance = { 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; @@ -243,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: InternalInstanceHandle, -): null | PublicInstance { +): null | PublicInstance | PublicTextInstance { + if (internalInstanceHandle.tag === HostText) { + const textInstance: TextInstance = internalInstanceHandle.stateNode; + return getPublicTextInstance(textInstance, internalInstanceHandle); + } + const instance: Instance = internalInstanceHandle.stateNode; return getPublicInstance(instance); } 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/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' { From 490bc086729ac8bc6f73e0a7da232c9cc26cc901 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20Norte?= Date: Thu, 30 Mar 2023 13:56:44 +0100 Subject: [PATCH 3/6] Add unit tests --- .../__tests__/ReactFabric-test.internal.js | 60 +++++++++++++++++++ 1 file changed, 60 insertions(+) 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..6b154ce244b3e 100644 --- a/packages/react-native-renderer/src/__tests__/ReactFabric-test.internal.js +++ b/packages/react-native-renderer/src/__tests__/ReactFabric-test.internal.js @@ -12,6 +12,7 @@ let React; let ReactFabric; +let ReactNativePrivateInterface; let createReactNativeComponentClass; let StrictMode; let act; @@ -39,6 +40,7 @@ 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; @@ -1035,4 +1037,62 @@ describe('ReactFabric', () => { const node = getNodeFromPublicInstance(viewRef); 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( + { + viewRef = ref; + }} + />, + 1, + ); + }); + + const internalInstanceHandle = nativeFabricUIManager.createNode.mock.calls[0][4]; + expect(internalInstanceHandle).toEqual(expect.any(Object)); + + 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); + }); }); From 6f9771ecf6e8c45140dbd8bdb759936561371be0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20Norte?= Date: Thu, 30 Mar 2023 13:57:05 +0100 Subject: [PATCH 4/6] Fix test for getNodeFromInternalInstanceHandle --- .../src/__tests__/ReactFabric-test.internal.js | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) 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 6b154ce244b3e..f423373cb5f6e 100644 --- a/packages/react-native-renderer/src/__tests__/ReactFabric-test.internal.js +++ b/packages/react-native-renderer/src/__tests__/ReactFabric-test.internal.js @@ -16,8 +16,6 @@ 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 " + @@ -44,11 +42,6 @@ describe('ReactFabric', () => { 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; }); @@ -939,7 +932,7 @@ 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 () => { @@ -976,7 +969,7 @@ 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 () => { @@ -1030,11 +1023,14 @@ describe('ReactFabric', () => { ); }); + 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 = getNodeFromPublicInstance(viewRef); + const node = ReactFabric.getNodeFromInternalInstanceHandle(internalInstanceHandle); expect(node).toBe(expectedShadowNode); }); From bd6c6e95c397cee00a18cab7f5aca930cc24934b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20Norte?= Date: Thu, 30 Mar 2023 14:07:21 +0100 Subject: [PATCH 5/6] Add definitions in ReactNativeTypes --- packages/react-native-renderer/src/ReactNativeTypes.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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, ... }; From c9ac0e3af0cf2159cea35769cfa009a13d4e8978 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20Norte?= Date: Thu, 30 Mar 2023 14:12:59 +0100 Subject: [PATCH 6/6] Fix lint/prettier --- .../__tests__/ReactFabric-test.internal.js | 63 +++++++++++-------- 1 file changed, 37 insertions(+), 26 deletions(-) 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 f423373cb5f6e..4e97cead6f349 100644 --- a/packages/react-native-renderer/src/__tests__/ReactFabric-test.internal.js +++ b/packages/react-native-renderer/src/__tests__/ReactFabric-test.internal.js @@ -932,7 +932,9 @@ describe('ReactFabric', () => { '\n in RCTView (at **)' + '\n in ContainsStrictModeChild (at **)', ]); - expect(match).toBe(ReactNativePrivateInterface.getNativeTagFromPublicInstance(child)); + expect(match).toBe( + ReactNativePrivateInterface.getNativeTagFromPublicInstance(child), + ); }); it('findNodeHandle should warn if passed a component that is inside StrictMode', async () => { @@ -969,7 +971,9 @@ describe('ReactFabric', () => { '\n in RCTView (at **)' + '\n in IsInStrictMode (at **)', ]); - expect(match).toBe(ReactNativePrivateInterface.getNativeTagFromPublicInstance(child)); + expect(match).toBe( + ReactNativePrivateInterface.getNativeTagFromPublicInstance(child), + ); }); it('should no-op if calling sendAccessibilityEvent on unmounted refs', async () => { @@ -1010,27 +1014,21 @@ describe('ReactFabric', () => { uiViewClassName: 'RCTView', })); - let viewRef; await act(() => { - ReactFabric.render( - { - viewRef = ref; - }} - />, - 1, - ); + ReactFabric.render(, 1); }); - const internalInstanceHandle = nativeFabricUIManager.createNode.mock.calls[0][4]; + 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); + const node = ReactFabric.getNodeFromInternalInstanceHandle( + internalInstanceHandle, + ); expect(node).toBe(expectedShadowNode); }); @@ -1053,10 +1051,14 @@ describe('ReactFabric', () => { ); }); - const internalInstanceHandle = nativeFabricUIManager.createNode.mock.calls[0][4]; + const internalInstanceHandle = + nativeFabricUIManager.createNode.mock.calls[0][4]; expect(internalInstanceHandle).toEqual(expect.any(Object)); - const publicInstance = ReactFabric.getPublicInstanceFromInternalInstanceHandle(internalInstanceHandle); + const publicInstance = + ReactFabric.getPublicInstanceFromInternalInstanceHandle( + internalInstanceHandle, + ); expect(publicInstance).toBe(viewRef); }); @@ -1069,26 +1071,35 @@ describe('ReactFabric', () => { })); await act(() => { - ReactFabric.render( - Text content, - 1, - ); + ReactFabric.render(Text content, 1); }); // Access the internal instance handle used to create the text node. - const internalInstanceHandle = nativeFabricUIManager.createNode.mock.calls[0][4]; + 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(); + expect( + ReactNativePrivateInterface.createPublicTextInstance, + ).not.toHaveBeenCalled(); - const publicInstance = ReactFabric.getPublicInstanceFromInternalInstanceHandle(internalInstanceHandle); + 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); + expect( + ReactNativePrivateInterface.createPublicTextInstance, + ).toHaveBeenCalledTimes(1); + expect( + ReactNativePrivateInterface.createPublicTextInstance, + ).toHaveBeenCalledWith(internalInstanceHandle); - const expectedPublicInstance = ReactNativePrivateInterface.createPublicTextInstance.mock.results[0].value; + const expectedPublicInstance = + ReactNativePrivateInterface.createPublicTextInstance.mock.results[0] + .value; expect(publicInstance).toBe(expectedPublicInstance); }); });