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

Move definition and instantiation of public instances for Fabric from React to React Native #26418

Conversation

rubennorte
Copy link
Contributor

@rubennorte rubennorte commented Mar 17, 2023

THIS IS A DRAFT

NOTE: the target branch of this PR is the branch with the changes in #26416 instead of main, so you can see what would be the changes in this PR after #26416 is merged.

Summary

This finalizes the refactor to move the definition and instantiation of public instances from react to react-native. See #26416 for additional context.

How did you test this change?

Will do a manual sync of the PR to React Native to test this in Meta's infra.

@rubennorte rubennorte changed the title Remove ReactFabricPublicInstance and used definition from ReactNative… Move definition and instantiation of public instances for Fabric from React to React Native Mar 17, 2023
sammy-SC pushed a commit that referenced this pull request Mar 20, 2023
…26416)

## Summary

We are going to move the definition of public instances from React to
React Native to have them together with the native methods in Fabric
that they invoke. This will allow us to have a better type safety
between them and iterate faster on the implementation of this proposal:
react-native-community/discussions-and-proposals#607

The interface between React and React Native would look like this after
this change and a following PR (#26418):

React → React Native:
```javascript
ReactNativePrivateInterface.createPublicInstance // to provide via refs
ReactNativePrivateInterface.getNodeFromPublicInstance // for DevTools, commands, etc.
ReactNativePrivateInterface.getNativeTagFromPublicInstance // to implement `findNodeHandle`
```

React Native → React (ReactFabric):
```javascript
ReactFabric.getNodeFromInternalInstanceHandle // to get most recent node to call into native
ReactFabric.getPublicInstanceFromInternalInstanceHandle // to get public instances from results from native
```

## How did you test this change?

Flow
Existing unit tests
@rubennorte rubennorte deleted the branch clean-interface-between-react-and-react-native March 20, 2023 14:06
@rubennorte rubennorte closed this Mar 20, 2023
@rubennorte rubennorte deleted the use-fabric-host-instance-from-react-native-internal-interface branch March 20, 2023 14:07
@rubennorte
Copy link
Contributor Author

Re-published in #26437.

rubennorte added a commit 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 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)
bigfootjon pushed a commit that referenced this pull request Apr 18, 2024
…26416)

## Summary

We are going to move the definition of public instances from React to
React Native to have them together with the native methods in Fabric
that they invoke. This will allow us to have a better type safety
between them and iterate faster on the implementation of this proposal:
react-native-community/discussions-and-proposals#607

The interface between React and React Native would look like this after
this change and a following PR (#26418):

React → React Native:
```javascript
ReactNativePrivateInterface.createPublicInstance // to provide via refs
ReactNativePrivateInterface.getNodeFromPublicInstance // for DevTools, commands, etc.
ReactNativePrivateInterface.getNativeTagFromPublicInstance // to implement `findNodeHandle`
```

React Native → React (ReactFabric):
```javascript
ReactFabric.getNodeFromInternalInstanceHandle // to get most recent node to call into native
ReactFabric.getPublicInstanceFromInternalInstanceHandle // to get public instances from results from native
```

## How did you test this change?

Flow
Existing unit tests

DiffTrain build for commit 3554c88.
bigfootjon pushed a commit that referenced this pull request Apr 18, 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 commit 9c54b29.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants