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

Added ReactFeatureFlags shim for React Native #11694

Merged
merged 2 commits into from
Nov 28, 2017

Conversation

bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Nov 28, 2017

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-added debugRenderPhaseSideEffects, which gets loaded at runtime via the ReactFeatureFlags module. This PR adds a missing shim for the ReactFeatureFlags module. (I should have included it with the previous PR but didn't.)

*
* 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.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MIT

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😆 Whoops! Thanks

@gaearon
Copy link
Collaborator

gaearon commented Nov 28, 2017

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.

@bvaughn
Copy link
Contributor Author

bvaughn commented Nov 28, 2017

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.

@gaearon
Copy link
Collaborator

gaearon commented Nov 28, 2017

To clarify, AFAIK the only reason we added "shims" to the build process at all is to remove all __INTERNAL access in RN repo. This allows us to know when it's safe to remove an internal property.

@bvaughn
Copy link
Contributor Author

bvaughn commented Nov 28, 2017

True!

We use shims for other things in RN too though, like ReactNative.

Copy link
Collaborator

@gaearon gaearon left a 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.

@gaearon
Copy link
Collaborator

gaearon commented Nov 28, 2017

We use shims for other things in RN too though, like ReactNative.

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.

@bvaughn
Copy link
Contributor Author

bvaughn commented Nov 28, 2017

Isn't this similar to ReactFeatureFlags-www.js?

@gaearon
Copy link
Collaborator

gaearon commented Nov 28, 2017

No. ReactFeatureFlags-www.js is more similar to ReactNativeFeatureFlags. Note how it requires the dynamic version inside of it. :-) The dynamic version exists in www. Because it depends on GK.

@bvaughn
Copy link
Contributor Author

bvaughn commented Nov 28, 2017

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.

@gaearon
Copy link
Collaborator

gaearon commented Nov 28, 2017

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.

@bvaughn
Copy link
Contributor Author

bvaughn commented Nov 28, 2017

I thought similarly- that we wouldn't want to change ReactNativeFeatureFlags without changing the shim. Which is why I thought it made sense to keep in GitHub rather than fbsource.

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.

@gaearon
Copy link
Collaborator

gaearon commented Nov 28, 2017

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.

@bvaughn
Copy link
Contributor Author

bvaughn commented Nov 28, 2017

Cool. Thanks for the discussion. 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants