-
Notifications
You must be signed in to change notification settings - Fork 46.4k
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
Remove dynamic modern event system flag for FB #19059
Conversation
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 f0a564d:
|
Details of bundled changes.Comparing: 1d85bb3...f0a564d react-dom
Size changes (experimental) |
Details of bundled changes.Comparing: 1d85bb3...f0a564d react-dom
Size changes (stable) |
@@ -16,6 +16,7 @@ const importSideEffects = Object.freeze({ | |||
'react-dom/server': HAS_NO_SIDE_EFFECTS_ON_IMPORT, | |||
'react/jsx-dev-runtime': HAS_NO_SIDE_EFFECTS_ON_IMPORT, | |||
'react-fetch/node': HAS_NO_SIDE_EFFECTS_ON_IMPORT, | |||
'react-dom': HAS_NO_SIDE_EFFECTS_ON_IMPORT, |
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.
Why is this new?
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.
We have a block of code in ReactTestUtils that is wrapped in the feature flag. Inside this block we reference an import from react-dom
. When we remove the block, we end up with having a dangling import to react-dom
at the top of the module without it being used. In turn, this causes Rollup to fail the build.
This will organically go away when we remove the flags entirely from OSS.
* Remove synamic modern event system flag for FB
This makes the modern event system flag enabled statically for FB bundles.