-
Notifications
You must be signed in to change notification settings - Fork 47.3k
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
Convert React Native builds to named exports #18136
Conversation
These don't need any forks because we always export the same things atm.
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 6930f7c:
|
Details of bundled changes.Comparing: 60016c4...6930f7c react-native-renderer
Size changes (experimental) |
Details of bundled changes.Comparing: 60016c4...6930f7c react-native-renderer
Size changes (stable) |
66be026
to
6930f7c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m not exactly sure from your summary what the change is here or what the purpose is. However, the resulting diff you linked seems fine if export syntax works, so that’s fine with me. Having a test plan that sanity checks that this build still works in the Facebook app would be great though!
@@ -137,7 +135,6 @@ export type ReactFabricType = { | |||
callback: ?Function, | |||
): any, | |||
unmountComponentAtNode(containerTag: number): any, | |||
__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED: SecretInternalsFabricType, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we still need to export an empty object here so that it matches the paper renderer. Of code reads a key off and checks if the value is defined, that would now throw in fabric right because the object would be undefined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The one current caller doesn't check if it's defined.
And we're not going to be adding this back. No more internals.
There's some more info in #18106 I think in a follow up I'll actually convert this to ES modules. Not sure if RN can take advantage of it though since it's a huge mix of ES modules and requires.
That's what the sync is for. We'll find out. :) |
That puts the burden on someone else, at some point in the future which would require them identifying this is the problem, and reverting it. However, reverting it probably wouldn't be trivial with additional changes stacked on top. Also, once this lands all partial syncs will have conflicts so we'd be totally blocked on moving forward until it can be resolved. Doing a test sync of a change like this and verifying that the change works seems like something that should be done before merging. |
I mean this is literally the point of the sync. To batch that burden on someone else. Every other thing we land has that problem. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll take the burden. :-)
These don't need any forks because we always export the same things atm.
They still build to CJS though atm.