-
Notifications
You must be signed in to change notification settings - Fork 47.1k
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
Added ReactFeatureFlags shim for React Native #11694
Added ReactFeatureFlags shim for React Native #11694
Conversation
* | ||
* This source code is licensed under the BSD-style license found in the | ||
* LICENSE file in the root directory of this source tree. An additional grant | ||
* of patent rights can be found in the PATENTS file in the same directory. |
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.
MIT
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.
😆 Whoops! Thanks
Do we need to add it to React repo at all? It seems to me like this can live in the RN repo (it will be synced there anyway). I don't have a strong opinion though. |
We don't need to, but it really belongs there I think, given how the shims work and how this file is coupled to the deferred runtime flag in the fork. |
To clarify, AFAIK the only reason we added "shims" to the build process at all is to remove all |
True! We use shims for other things in RN too though, like |
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 don't mind this, but it's not symmetrical with how we do it in www (which has its own ReactFeatureFlags
in www source, rather than in our repo). I guess if we rename both to ReactDynamicFeatureFlags
in the future maybe it will be clearer.
AFAIK the initial plan was to delete them asap. We just never did it. We tried to keep the Rollup PR simple and require little to not changes in RN repo for next sync. So we had a file for every Haste require that would go missing. |
Isn't this similar to |
No. |
Yeah, I just noticed that, although it's also in the "shims" sub-dir... Hm. I still think b'c of the RN sync workflow (React GitHub -> fbsource -> ReactNative GitHub) it makes slightly more sense to put the file here. |
Let's agree to disagree 😛 I don't think there's a single good answer here. I do think that we'll never want to change this file without changing its cousin in fbsource though. So my natural instinct is to put its source of truth closer to fbsource (RN GH) than away from it (here). Especially since we don't actually depend on this file for our purposes in this repo. I would also prefer to keep www and RN workflows more symmetric. |
I thought similarly- that we wouldn't want to change I agree that there's not an obvious best answer here. Agree to disagree then. 😄 We can always revisit later if one approach becomes more obviously correct. |
Again, I don't feel strongly about it though. Takes a few iterations to find a good balance. Maybe let's try it this way. |
Cool. Thanks for the discussion. 😄 |
PR #11603 added a feature-flag-fork for the React Native renderer, dubbed
ReactNativeFeatureFlags
. This fork uses statically-defined flags for everything but the recently-addeddebugRenderPhaseSideEffects
, which gets loaded at runtime via theReactFeatureFlags
module. This PR adds a missing shim for theReactFeatureFlags
module. (I should have included it with the previous PR but didn't.)