rename yoga podspec to ReactYoga to prevent clash with YogaKit

[koenpunt]

Summary

Adding YogaKit to a brownfield React Native project caused a name clash, because Yoga, a dependency of YogaKit did install in the same folder as the yoga library that comes with React Native, which made it impossible to use YogaKit in an React Native app.
By renaming the bundled yoga podspec they no longer install in the same directory.

I might be unaware of other ways of preventing this name clash on a project level, but I figured this would be the most robust solution for everybody.

Fixes #20651

Test Plan:

This is change that affects the build process, so if the build still succeeds it should still work, but additionally I will build and run the RNTester app and see if that still works. And for the cocoapods implementation I will create a sample app that integrates React Native and YogaKit, and will verify that it still works.

Release Notes:

[IOS] [Fixed] - Prevent name clash with YogaKit and Yoga

[koenpunt]

While following my test plan, I found out that the import statements for yoga are now different for when using CocoaPods. I assumed that since a module_name was set in the podspec, this name would be honored, but apparently this is not the case.

1 possible solution is renaming yoga to ReactYoga everywhere. (currently trying that here: https://github.com/koenpunt/react-native/tree/yoga-cp-naming-dev)

Another solution would be that RN is going to use the official Yoga source, but then implementing projects have to use the same version of Yoga, which possibly will be lagging behind.

[koenpunt]

I've added a commit that updates all the imports for yoga to be conditional, which in my testing works with all possible configurations; react-native init, CocoaPods, and CocoaPods with use_frameworks!.

I've also created an example app that demonstrates the use of YogaKit in conjunction with React Native: https://github.com/koenpunt/ReactYogaExample

[orta]

It's bit of a breaking change - existing consumers may need to change yoga to YogaReact. It does however solve one of our blockers for deployment ( artsy/emission#1077 (comment) ) )

However, I think the idea is in the right place. This doc will also need an update.

👍

[orta]

Worth noting that these changes would likely stay this way in a post- react-native-community/discussions-and-proposals#18 world too.

[koenpunt]

@orta thanks for chiming in! Would really see this change happen, because currently I'm rolling with patched version of RN. Also nice to see your proposal regarding RN and CocoaPods 👍

[koenpunt]

And I've already submitted a PR for the documentation change for when this gets merged, hopefully, eventually: facebook/react-native-website#521

[EternalChildren]

i don't see these modifications in master branch. so who can tell me the bug is already fixed?

[cpojer]

What's the status of this PR?

cc @davidaurelio

[koenpunt]

I'll bring it up-to-date with master somewhere in the coming hours

[cpojer]

@koenpunt are you planning on doing the update?

[davidaurelio]

@cpojer technically, this should be fine, internally this podspec is separate from Yoga’s. Not sure what the best way for apps with multiple deps on Yoga really is, though. Can this cause linker problems?

[cpojer]

@davidaurelio ah, that makes sense, thank you. In that case I think we should go ahead and merge this PR once @koenpunt rebases.

[cpojer]

cc @fson

[koenpunt]

I've rebased once more and squashed the commits into one.

[koenpunt]

Since now all podspecs have been split up with names like React-ART, should I maybe name this React-yoga instead of ReactYoga?

[orta]

Yeah, I think so, good idea

[koenpunt]

Should be good now. I've also removed the .React suffix from the version, since the spec name now already includes the React- prefix.

[cpojer]

Let's ship it.

[facebook-github-bot]

@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[davidaurelio]

@koenpunt We’d very much like to not have this include switch in YGJNI.cpp, and Yoga’s code base as a whole.

• Yoga is used by different OSS projects. I would rather not set a precedent to special-case one of them.
• Other projects using Yoga want to continue to work with compilers that don’t implement __has_include(…)
• There is no test that would stop Yoga contributors from removing that if/else.

Can we isolate the solution to the cocoapods level? I am not sure whether this is going to work, but what about:

• organising the React-yoga pod such that it has an extra yoga/ subdirectory
• use spec.header_search_paths (example) in React Native’s pod file(s).
• continue to use #include <yoga/…> everywhere.

Speaking of having two versions of Yoga in a project: how does that work in the end when everything is linked?

[koenpunt]

We’d very much like to not have this include switch in YGJNI.cpp, and Yoga’s code base as a whole.

I wasn't aware this was an imported library, I assumed they are all local sources.

Speaking of having two versions of Yoga in a project: how does that work in the end when everything is linked?

From my experience this wasn't a problem.

But in the end it would of course be better if RN would just use the published version of yoga, instead of the bundled one. Not sure why this wasn't possible, but I believe it has to do with the fact that Facebook doesn't use cocoapods, so there are different releases used internally than those that are published to cocoapods.

[davidaurelio]

I wasn't aware this was an imported library, I assumed they are all local sources.

Sorry about that – it isn’t really obvious that internally we just mirror Yoga into RN, even though they go to different repositories.

But in the end it would of course be better if RN would just use the published version of yoga, instead of the bundled one. Not sure why this wasn't possible, but I believe it has to do with the fact that Facebook doesn't use cocoapods, so there are different releases used internally than those that are published to cocoapods.

The internal setup might not be the problem. Maybe the RN team would like to have an extra copy to be able to make changes without releasing new versions. That would be another valid way of doing things.

Is there any chance that my suggestion is going to work out? That seems to be a way to clean up without introducing too many changes at once.

[fson]

• use spec.header_search_paths (example) in React Native’s pod file(s).

I think it would be best to avoid using the pod_target_xcconfig to set more HEADER_SEARCH_PATHS entries, if possible. I know some dependencies of React are already compiled like this, for example Folly. (My git archeology shows this was initially introduced here and described as a "pretty ugly hack"). Doing this seems to fully subvert how CocoaPods deals with dependencies and compiles them. The end result of this is that now all podspecs directly or indirectly depending on Folly must not only add Folly to header search path, must also add its dependencies to the header search path and specify any compiler flags used to compile it (and its dependencies).

When not using the header search path hack, I believe the React-Yoga podspec can just set spec.header_dir = 'yoga' and then all pods that depend on it can still use #include <yoga/…>.

[orta]

👋 - took a look at this again, I'm a little confused about how two copies of Yoga can ever co-exist in the app? Native code shouldn't allow for two versions of the same library without some pretty hectic code changes.

Is it possible that instead we could loosen the YogaKit Podspec dependency to allow either the React version, or a version that is shipped directly from their repo?

[cpojer]

It seems like we are unlikely to move forward with this PR so I'm going to close it.

@koenpunt if you are still interested in resolving this issue, please check out the latest comments by @davidaurelio @fson and @orta and send a new PR. Thank you!