-
Notifications
You must be signed in to change notification settings - Fork 45.6k
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 dead code elimination for feature flags #11453
Conversation
May i ask: why there are some special care of |
www is Facebook.com codebase. That's where we develop React. Historically it's been a playground for our new features since it lets us get feedback from product engineers without committing to supporting a particular API, and ensure new features are solid before they are pushed into open source. There's nothing preventing you from using our custom builds too, if that's something you want to do. |
Turning flags into named exports fixes dead code elimination. This required some restructuring of how we verify that flag types match up. I used the Check<> trick combined with import typeof, as suggested by @calebmer. For www, we can no longer re-export `require('ReactFeatureFlags')` directly, and instead destructure it. This means flags have to be known at init time. This is already the case so it's not a problem. In fact it may be better since it removes extra property access in tight paths. For things that we *want* to be dynamic on www (currently, only performance flag) we can export a function to toggle it, and then put it on the secret exports. In fact this is better than just letting everyone mutate the flag at arbitrary times since we can provide, e.g., a ref counting interface to it.
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.
LGTM
* Fix dead code elimination for feature flags Turning flags into named exports fixes dead code elimination. This required some restructuring of how we verify that flag types match up. I used the Check<> trick combined with import typeof, as suggested by @calebmer. For www, we can no longer re-export `require('ReactFeatureFlags')` directly, and instead destructure it. This means flags have to be known at init time. This is already the case so it's not a problem. In fact it may be better since it removes extra property access in tight paths. For things that we *want* to be dynamic on www (currently, only performance flag) we can export a function to toggle it, and then put it on the secret exports. In fact this is better than just letting everyone mutate the flag at arbitrary times since we can provide, e.g., a ref counting interface to it. * Record sizes
Turning flags into named exports fixes the regression in dead code elimination (#11450).
This required some restructuring of how we verify that flag types match up. I used the
Check<>
trick combined withimport typeof
, as suggested by @calebmer.For www, we can no longer re-export
require('ReactFeatureFlags')
directly, and instead destructure it. This means flags have to be known at init time. This is already the case so it's not a problem. In fact it may be better since it removes extra property access in tight paths.For things that we want to be dynamic on www (currently, only performance flag which I haven't added yet) we can export a function to toggle it, and then put it on the secret exports. In fact this is better than just letting everyone mutate the flag at arbitrary times since we can provide, e.g., a ref counting interface to it.