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

fix: add isChildPublicInstance to ReactNativeTypes #27788

Conversation

hoxyq
Copy link
Contributor

@hoxyq hoxyq commented Dec 4, 2023

Follow-up on #27783.

React Native is actually using ReactNativeTypes, which are synced from this repo. In order to make isChildPublicInstance visible for renderers inside React Native repository, we need to list it in ReactNativeTypes.

Because of current circular dependency between React Native and React, it is impossible to actually type it properly:

  • Can't import any types in ReactNativeTypes from local files, because it will break React Native, once synced.
  • Implementations can't use real types in their definitions, because it will break these checks:

// Assert that the exports line up with the type we're going to expose.
(ReactFabric: ReactFabricType);

// Assert that the exports line up with the type we're going to expose.
// eslint-disable-next-line ft-flow/no-unused-expressions
(ReactNative: ReactNativeType);

@hoxyq hoxyq requested a review from rubennorte December 4, 2023 19:36
@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Dec 4, 2023
@react-sizebot
Copy link

react-sizebot commented Dec 4, 2023

Comparing: 223db40...705e11b

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js = 175.90 kB 175.90 kB = 54.75 kB 54.75 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 177.97 kB 177.97 kB = 55.39 kB 55.39 kB
facebook-www/ReactDOM-prod.classic.js = 569.81 kB 569.81 kB = 100.29 kB 100.29 kB
facebook-www/ReactDOM-prod.modern.js = 553.67 kB 553.67 kB = 97.38 kB 97.38 kB
react-native/shims/ReactNativeTypes.js +2.72% 8.12 kB 8.34 kB +2.28% 2.24 kB 2.29 kB
test_utils/ReactAllWarnings.js Deleted 67.17 kB 0.00 kB Deleted 16.35 kB 0.00 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
react-native/shims/ReactNativeTypes.js +2.72% 8.12 kB 8.34 kB +2.28% 2.24 kB 2.29 kB
test_utils/ReactAllWarnings.js Deleted 67.17 kB 0.00 kB Deleted 16.35 kB 0.00 kB

Generated by 🚫 dangerJS against 705e11b

@hoxyq hoxyq force-pushed the react-native/update-react-native-types-with-isChildPublicInstance branch from b1e1e10 to 55a31ac Compare December 5, 2023 12:07
@hoxyq hoxyq force-pushed the react-native/update-react-native-types-with-isChildPublicInstance branch from 55a31ac to 705e11b Compare December 5, 2023 12:20
@hoxyq hoxyq merged commit c29ca23 into facebook:main Dec 5, 2023
36 checks passed
@hoxyq hoxyq deleted the react-native/update-react-native-types-with-isChildPublicInstance branch December 5, 2023 13:01
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
Follow-up on facebook#27783.

React Native is actually using `ReactNativeTypes`, which are synced from
this repo. In order to make `isChildPublicInstance` visible for
renderers inside React Native repository, we need to list it in
`ReactNativeTypes`.

Because of current circular dependency between React Native and React,
it is impossible to actually type it properly:
- Can't import any types in `ReactNativeTypes` from local files, because
it will break React Native, once synced.
- Implementations can't use real types in their definitions, because it
will break these checks:


https://github.com/facebook/react/blob/223db40d5a04dc3311f963f5296675f7f43139e8/packages/react-native-renderer/fabric.js#L12-L13


https://github.com/facebook/react/blob/223db40d5a04dc3311f963f5296675f7f43139e8/packages/react-native-renderer/index.js#L12-L14
bigfootjon pushed a commit that referenced this pull request Apr 18, 2024
Follow-up on #27783.

React Native is actually using `ReactNativeTypes`, which are synced from
this repo. In order to make `isChildPublicInstance` visible for
renderers inside React Native repository, we need to list it in
`ReactNativeTypes`.

Because of current circular dependency between React Native and React,
it is impossible to actually type it properly:
- Can't import any types in `ReactNativeTypes` from local files, because
it will break React Native, once synced.
- Implementations can't use real types in their definitions, because it
will break these checks:

https://github.com/facebook/react/blob/223db40d5a04dc3311f963f5296675f7f43139e8/packages/react-native-renderer/fabric.js#L12-L13

https://github.com/facebook/react/blob/223db40d5a04dc3311f963f5296675f7f43139e8/packages/react-native-renderer/index.js#L12-L14

DiffTrain build for commit c29ca23.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants