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.

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

Bring ReactFabricHostComponent back to react-native #36570

Closed
wants to merge 1 commit into from

Conversation

rubennorte
Copy link
Contributor

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:

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

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 facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Facebook Partner: Facebook Partner fb-exported labels Mar 22, 2023
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D43772356

@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,519,151 +2,402
android hermes armeabi-v7a 7,835,230 +2,383
android hermes x86 8,999,161 +2,395
android hermes x86_64 8,854,318 +2,385
android jsc arm64-v8a 9,140,510 +1,234
android jsc armeabi-v7a 8,332,650 +1,227
android jsc x86 9,195,218 +1,236
android jsc x86_64 9,453,168 +1,233

Base commit: 7410746
Branch: main

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Mar 22, 2023
@facebook-github-bot
Copy link
Contributor

This pull request has been merged in ee76107.

@rubennorte rubennorte deleted the export-D43772356 branch March 22, 2023 13:58
rubennorte added a commit to facebook/react that referenced this pull request Mar 22, 2023
…PrivateInterface (#26437)

## Summary

Now that React Native owns the definition for public instances in Fabric
and ReactNativePrivateInterface provides the methods to create instances
and access private fields (see
facebook/react-native#36570), we can remove the
definitions from React.

After this PR, React Native public instances will be opaque types for
React and it will only handle their creation but not their definition.
This will make RN similar to DOM in how public instances are handled.

This is a new version of #26418 which was closed without merging.

## How did you test this change?

* Existing tests.
* Manually synced the changes in this PR to React Native and tested it
end to end in Meta's infra.
github-actions bot pushed a commit to facebook/react that referenced this pull request Mar 22, 2023
…PrivateInterface (#26437)

## Summary

Now that React Native owns the definition for public instances in Fabric
and ReactNativePrivateInterface provides the methods to create instances
and access private fields (see
facebook/react-native#36570), we can remove the
definitions from React.

After this PR, React Native public instances will be opaque types for
React and it will only handle their creation but not their definition.
This will make RN similar to DOM in how public instances are handled.

This is a new version of #26418 which was closed without merging.

## How did you test this change?

* Existing tests.
* Manually synced the changes in this PR to React Native and tested it
end to end in Meta's infra.

DiffTrain build for [9c54b29](9c54b29)
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
jerrydev0927 added a commit to jerrydev0927/react that referenced this pull request Jan 5, 2024
…PrivateInterface (#26437)

## Summary

Now that React Native owns the definition for public instances in Fabric
and ReactNativePrivateInterface provides the methods to create instances
and access private fields (see
facebook/react-native#36570), we can remove the
definitions from React.

After this PR, React Native public instances will be opaque types for
React and it will only handle their creation but not their definition.
This will make RN similar to DOM in how public instances are handled.

This is a new version of #26418 which was closed without merging.

## How did you test this change?

* Existing tests.
* Manually synced the changes in this PR to React Native and tested it
end to end in Meta's infra.

DiffTrain build for [9c54b29b44d24f8f8090da9c7ebf569747a444df](facebook/react@9c54b29)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported Merged This PR has been merged. p: Facebook Partner: Facebook Partner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants