Extracted definition and access to public instances to a separate module in Fabric #26291
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…in the Fabric version of GlobalResponderHandler
|
Comparing: b72ed69...b2a5c00 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
rubennorte
force-pushed
the
extract-react-fabric-public-instance-handling-to-module
branch
from
75e7db1 to
752e093
Compare
rubennorte
force-pushed
the
extract-react-fabric-public-instance-handling-to-module
branch
2 times, most recently
from
02238bc to
53f270a
Compare
rubennorte
commented
Mar 3, 2023
| @@ -45,26 +45,6 @@ export function mountSafeCallback_NOT_REALLY_SAFE( | |||
| }; | |||
| } | |||
|
|
|||
| export function throwOnStylesProp(component: any, props: any) { | |||
There was a problem hiding this comment.
This is unused :)
rubennorte
force-pushed
the
extract-react-fabric-public-instance-handling-to-module
branch
3 times, most recently
from
d06a621 to
5887cde
Compare
rubennorte
force-pushed
the
extract-react-fabric-public-instance-handling-to-module
branch
from
5887cde to
b2a5c00
Compare
|
I realized I didn't migrate some logic in |
fkgozali
reviewed
Mar 3, 2023
| // $FlowFixMe Flow has hardcoded values for React DOM that don't work with RN | ||
| return componentOrHandle; | ||
|
|
||
| // For compatibility with Paper |
There was a problem hiding this comment.
nit: avoid codename "Paper", use "legacy renderer"
fkgozali
reviewed
Mar 3, 2023
| fromOrToStateNode && fromOrToStateNode.canonical._internalInstanceHandle | ||
| ); | ||
|
|
||
| if (isFabric) { |
There was a problem hiding this comment.
is this safe removal because this handler file is solely handling fabric? no mixed case?
| nativeTag: number, | ||
| viewConfig: ViewConfig, | ||
| currentProps: Props, | ||
| internalInstanceHandle: Object, |
There was a problem hiding this comment.
nit: a bit redundant name with internal prefix?
instance.internals.internalInstanceHandle
what about instance.internals.instanceHandle?
| publicInstance: ReactFabricHostComponent, | ||
| // We define this as an object instead of as separate fields to simplify | ||
| // making copies of instance where `internals` don't change. | ||
| internals: { |
There was a problem hiding this comment.
I'm not sure what the convention is - should this one be prefixed by _ ?
rubennorte
deleted the
extract-react-fabric-public-instance-handling-to-module
branch
|
Sorry I accidentally deleted the branch when restructuring my repo. I'll create another PR with these changes. |
rubennorte
added a commit
to rubennorte/react-native
that referenced
this pull request
Mar 22, 2023
Summary: I'm doing some preparations to implement this proposal to bring some DOM APIs to React Native refs: react-native-community/discussions-and-proposals#607 To make it easier to iterate on the proposal, and to improve the separation of concerns between React and React Native, I'm moving the definition of `ReactFabricHostComponent` (the public instance provided by React when using refs on host conmponents) to the `react-native` package. I already did some steps in the React repository to simplify this: * Removing unused imperative events that caused increased coupling: facebook/react#26282 * Extracting the definition of the public instance to a separate module: facebook/react#26291 In this case, in order to be able to move the definition from React to React Native, we need to: 1. Create the definition in React Native and export it through `ReactNativePrivateInterface`. 2. Update React to use that definition instead of the one in its own module. This diff implements the first step. `ReactNativeAttributePayload` is required by this definition and by the one for Paper that still exists in React. I moved it here so we only define it where we use it when we remove Paper. Paper will access it through `ReactNativePrivateInterface` as well. That will also allow us to remove a few other fields in that interface. Changelog: [Internal] Reviewed By: yungsters Differential Revision: D43772356 fbshipit-source-id: 0540fb9c665a9daa4d978926e0d01081f8541ffe
facebook-github-bot
pushed a commit
to facebook/react-native
that referenced
this pull request
Mar 22, 2023
Summary: Pull Request resolved: #36570 I'm doing some preparations to implement this proposal to bring some DOM APIs to React Native refs: react-native-community/discussions-and-proposals#607 To make it easier to iterate on the proposal, and to improve the separation of concerns between React and React Native, I'm moving the definition of `ReactFabricHostComponent` (the public instance provided by React when using refs on host conmponents) to the `react-native` package. I already did some steps in the React repository to simplify this: * Removing unused imperative events that caused increased coupling: facebook/react#26282 * Extracting the definition of the public instance to a separate module: facebook/react#26291 In this case, in order to be able to move the definition from React to React Native, we need to: 1. Create the definition in React Native and export it through `ReactNativePrivateInterface`. 2. Update React to use that definition instead of the one in its own module. This diff implements the first step. `ReactNativeAttributePayload` is required by this definition and by the one for Paper that still exists in React. I moved it here so we only define it where we use it when we remove Paper. Paper will access it through `ReactNativePrivateInterface` as well. That will also allow us to remove a few other fields in that interface. Changelog: [Internal] bypass-github-export-checks Reviewed By: yungsters Differential Revision: D43772356 fbshipit-source-id: 78dac152f415f19316ec90887127bf9861fe3110
jeongshin
pushed a commit
to jeongshin/react-native
that referenced
this pull request
May 7, 2023
Summary: Pull Request resolved: facebook#36570 I'm doing some preparations to implement this proposal to bring some DOM APIs to React Native refs: react-native-community/discussions-and-proposals#607 To make it easier to iterate on the proposal, and to improve the separation of concerns between React and React Native, I'm moving the definition of `ReactFabricHostComponent` (the public instance provided by React when using refs on host conmponents) to the `react-native` package. I already did some steps in the React repository to simplify this: * Removing unused imperative events that caused increased coupling: facebook/react#26282 * Extracting the definition of the public instance to a separate module: facebook/react#26291 In this case, in order to be able to move the definition from React to React Native, we need to: 1. Create the definition in React Native and export it through `ReactNativePrivateInterface`. 2. Update React to use that definition instead of the one in its own module. This diff implements the first step. `ReactNativeAttributePayload` is required by this definition and by the one for Paper that still exists in React. I moved it here so we only define it where we use it when we remove Paper. Paper will access it through `ReactNativePrivateInterface` as well. That will also allow us to remove a few other fields in that interface. Changelog: [Internal] bypass-github-export-checks Reviewed By: yungsters Differential Revision: D43772356 fbshipit-source-id: 78dac152f415f19316ec90887127bf9861fe3110
OlimpiaZurek
pushed a commit
to OlimpiaZurek/react-native
that referenced
this pull request
May 22, 2023
Summary: Pull Request resolved: facebook#36570 I'm doing some preparations to implement this proposal to bring some DOM APIs to React Native refs: react-native-community/discussions-and-proposals#607 To make it easier to iterate on the proposal, and to improve the separation of concerns between React and React Native, I'm moving the definition of `ReactFabricHostComponent` (the public instance provided by React when using refs on host conmponents) to the `react-native` package. I already did some steps in the React repository to simplify this: * Removing unused imperative events that caused increased coupling: facebook/react#26282 * Extracting the definition of the public instance to a separate module: facebook/react#26291 In this case, in order to be able to move the definition from React to React Native, we need to: 1. Create the definition in React Native and export it through `ReactNativePrivateInterface`. 2. Update React to use that definition instead of the one in its own module. This diff implements the first step. `ReactNativeAttributePayload` is required by this definition and by the one for Paper that still exists in React. I moved it here so we only define it where we use it when we remove Paper. Paper will access it through `ReactNativePrivateInterface` as well. That will also allow us to remove a few other fields in that interface. Changelog: [Internal] bypass-github-export-checks Reviewed By: yungsters Differential Revision: D43772356 fbshipit-source-id: 78dac152f415f19316ec90887127bf9861fe3110
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
The current definition of
Instancein Fabric has 2 fields:node: reference to the native node in the shadow tree.canonical: public instance provided to users via refs.We're currently using
canonicalnot only as the public instance, but also to store internal properties that Fabric needs to access in different parts of the codebase. Those properties are, in fact, available through refs as well, which breaks encapsulation.This PR splits that into 2 separate fields, leaving the definition of instance as:
node: reference to the native node in the shadow tree.publicInstance: public instance provided to users via refs.internals: collection of properties that are required by Fabric itself, given references to the instance.This also migrates all the current usages of
canonicalto use the right property depending on the use case.To improve encapsulation (and in preparation for the implementation of this proposal to bring some DOM APIs to public instances in React Native), this also moves the creation of and the access to the public instance to a separate module. In a following diff, that module will be moved into the
react-nativerepository and we'll access it throughReactNativePrivateInterface.How did you test this change?
Existing unit tests.
NOTE: This includes the changes in #26290. I'll rebase this onto main when that is merged.