Fix compatibility with CocoaPods frameworks

[jtreanor]

Summary

This fixes #25349. It allows React and the related pods to be integrated into Podfiles that use use_frameworks!. Some more background is explained in the issue.

The fix involves a number of changes:

• All podspecs now have different values for s.header_dir to avoid conflicts with #import <React/X> imports. This may be a breaking change if these are imported externally.
• Made C++ headers in React-Core private by default so that ObjC files can import the module without failures.
• Reduced the number of yoga headers that are exposed for the same reason as above. As far as I can see this doesn't cause issues but we can find another solution if it does.
• Adding some missing dependencies to fix undefined symbols errors.
• Added DoubleConversion to HEADER_SEARCH_PATHS where it was missing.

Note: The React-RCTFabric and React-Fabric pods are still incompatible with use_frameworks!, but work fine without it. Some more updates will be needed there.

Changelog

[iOS] [Fixed] - Fixed compatibility with CocoaPods frameworks.

Test Plan

Everything should work exactly as before, where use_frameworks! is not in Podfiles. I have a branch on my sample project here which has use_frameworks! in its Podfile to demonstrate this is fixed.

You can see that it works with these steps:

1. git clone git@github.com:jtreanor/react-native-cocoapods-frameworks.git
2. git checkout fix-frameworks
3. cd ios && pod install
4. cd .. && react-native run-ios

The sample app will build and run successfully. To see that it still works without frameworks, remove use_frameworks! from the Podfile and do steps 3 and 4 again.

[jtreanor]

I can see there are some failures due to the changed imports. I will update them shortly.

Update: Done now in 7ab5c34. I feel like there may be a better solution though.

[kelset]

Hey James!

Thanks for opening the PR 🙇‍♂️

It looks quite the big PR - even if the changes are few per file - so we'll try to get some extra people to take a close look.

[orta]

This should use React/RCTAnimationUtils.h I think?

[jtreanor]

You're right and I just pushed as you commented this 😄 Thanks!

[orta]

What's going on with this change?

[orta]

Ah, found the discussion at the top

[jtreanor]

This reverts a change that was made in #23803 which admittedly I don't have full context on.

The reason it is needed is that with all of the headers public, the module fails to build when imported into RCTImageSource.m with the error below because the headers include C++ code.

▸ Compiling RCTImageSource.m

❌  /Users/james/src/tmp/AwesomeProject/node_modules/react-native/ReactCommon/yoga/yoga/CompactValue.h:11:10: 'cmath' file not found

#include <cmath>
           ^~~~~~~~~~~~~~~~~~~~~~~~~



❌  /Users/james/src/tmp/AwesomeProject/node_modules/react-native/React/Base/RCTConvert.h:17:9: could not build module 'yoga'

#import <yoga/Yoga.h>
         ^~~~~~~

[orta]

Cool, I think the fundamentals here are good.

Any chance you could give an example of what someone would have had to have been doing to see this?

This may be a breaking change if these are imported externally

( One advantage is that this setup for Podspecs only came out in 0.60 (I think? dang, this has been a long release )

[jtreanor]

Thanks, @orta!

Any chance you could give an example of what someone would have had to have been doing to see this?

Sure. As a result of this change any user code that imports the non-core modules would need to be updated.

For example, if someone is using #import <React/RCTPushNotificationManager.h> as in the docs, that would need to be updated to #import <RCTPushNotification/RCTPushNotificationManager.h> after this change.

[shergin]

@ericlewis Eric, any thoughts?

[jtreanor]

Just a quick note about the breaking change mentioned above. The timing is probably reasonable since there are other breaking changes in 0.60.0.

Although I still think its the best thing to do, its worth pointing out an alternative. We could add a script_phase to each podspec that looks something like this:

s.script_phase = { :name => 'Copy Headers',
                   :execution_position => :before_compile
                   :script => 'DEST="${BUILT_PRODUCTS_DIR}/../include/React"; mkdir -p "$DEST"; find "${PODS_TARGET_SRCROOT}" -name "*.h" -exec cp {} "$DEST" \;',
                 }

Its a little hacky, but it would allow all existing imports to continue to work. <React/X> would no longer refer to a module, but just the include directory. I have a definite preference for the way it is on the PR right now but I wanted to share my thoughts.

[jtreanor]

@kelset I just saw your comment about releasing the latest RC.

I totally understand the core team have so many other things to do. However, given that this PR proses breaking changes, it would be really great if it could be included in 0.60.0. Let me know if there is anything I can do to help make this happen.

[kelset]

Hey James - yes, I fully agree with you. And I'd love to see it in 0.60.0 but it seems that internally FB has been working on things that would be touched by this PR, and they want to see those land before this... which means more delays for 0.60.0.

I'll try to raise it once more but honestly can't promise anything, it's not something I have full control over 😓

[jtreanor]

@kelset That's completely fair. Thanks for the update and all your help!

[hramos]

Hey @jtreanor, this PR was mostly blocked on some work I've been doing on RNTesterPods to ensure we can run these tests on our internal CI. I asked folks to hold off on touching RNTesterPods to avoid some nasty conflicts when we landed this internally. As it happens, we had to revert the change due to an issue related to what you're fixing on this PR, so let's just go ahead and get this merged. I'll rebase my changes on top of this. Thanks for fixing this!

[facebook-github-bot]

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

[hramos]

I should also say that, in my opinion, these changes meet the bar for a 0.60-RC cherry-pick as they fix an issue introduced in 0.60 itself. My upcoming changes mostly focus on moving the iOS tests from RNTester to RNTesterPods, so we should not block 0.60 on getting my own RNTesterPods changes in.

[fkgozali]

All podspecs now have different values for s.header_dir to avoid conflicts with #import <React/X> imports. This may be a breaking change if these are imported externally.

Question: is the issue here header namespace collisions across podspecs? I think in the ideal world we'd like most namespace header to be just React/ for consistency and ease of use.

This PR will require a lot of changes in various BUCK files we have today (to fix the namespace), and unfortunately they are not mapped 1-to-1 (in dir structure) with .podspecs yet (fixable, but requires bunch of refactoring unrelated to this PR). So I think it'll take some time to land this PR. We're looking into it.

[ericlewis]

My biggest concern is wether or not well badly break 3rd party libs. Do we have an assessment on what that could cause? From reading the issue about headers, it’ll take time to propagate, but libs will be actively updating too. This may result in major version changes across many libs and it should be considered. How prepared are we to do that, and is there an easy upgrade path for the libs that dont have the resources to move as quickly as the rest of us.

[ericlewis]

And to add to @fkgozali this in my opinion would probably break all of OSS fabric & I’m not exactly sure how to fix it. The library is extremely reliant on very specific naming structure. But it’s also not an accidental feature. This forces scary constraints and seemingly a divide.

[jtreanor]

Thanks for your input @fkgozali and @ericlewis.

Question: is the issue here header namespace collisions across podspecs? I think in the ideal world we'd like most namespace header to be just React/ for consistency and ease of use.

Yes, that is exactly the issue. Since RN is now split across several podspecs, Xcode builds them each as a separate framework and the headers live separately in each. With s.header_dir = "React" set on each podspec, as is the case on master, every podspec will be built as React.framework. When the compiler encounters something like #import <React/RCTDefines.h>, these conflict and it doesn't know where to look.

My biggest concern is wether or not well badly break 3rd party libs. Do we have an assessment on what that could cause? From reading the issue about headers, it’ll take time to propagate, but libs will be actively updating too. This may result in major version changes across many libs and it should be considered. How prepared are we to do that, and is there an easy upgrade path for the libs that dont have the resources to move as quickly as the rest of us.

This is a very valid point and I don't have a good answer for the scale of impact on third party libs. As it stands, any header imported from React-Core would continue to work fine but others would need to be changed.

As I mentioned above, there may be a hacky solution to merging all the headers into one namespace, but its not ideal. There may be another way to achieve this and I'm very open to suggestions.

Its worth clarifying that this PR fixes importing RN from Swift files. As it stands on master, import React from Swift will not work, for the same reason as above.

And to add to @fkgozali this in my opinion would probably break all of OSS fabric & I’m not exactly sure how to fix it.

Can you clarify what you mean by this? I'm not very familiar with Fabric but as far as I can tell, everything in the repo still builds fine as I have updated the required imports.

[fkgozali]

I looked into the actual issue today to understand the problem better:

Problem 1: Colliding .framework names

• Mainly React.framework because we hardcode header_dir to React everywhere.
• According to an Apple doc [1], header lookup is in the form of #include <Framework_name/Header_filename.h>
• It may be possible to trick all these .framework files to look for more header search paths, but I don't think that's right
• In CocoaPods, the .framework name defaults to header_dir, or module_name if provided [2]

Given that we have a lot of #import <React/*.h> in the codebase, it is best to preserve this namespace as much as possible. That means, all headers from React-Core.podspec should be preserved with React prefix to minimize breakages.

For the rest of the .podspec's, I think we'll have to adapt the new namespace (e.g. like what Fabric did). This could be breaking change, but I'm hoping it's minimized since it's not the React-Core lib.

Problem 2: Incomplete Dependencies in the podspec

Some of our .podspecs are missing direct dependencies. E.g. React-Core is missing deps to React-jsi and glog. This should be fixed regardless, and should be safe to do so. Parts of this current PR should be split into a separate PRs to fix these deps.

Problem 3: Something broke this in v0.60

Based on #25349, it seems like something broke this in v0.60. I'm not sure which commits caused it, but it would be best for 0.60 dot release (?) to revert the offending commits. @jtreanor do you know?

Then, we can fix things forward in master.

cc @hramos @cpojer

[1] https://developer.apple.com/library/archive/documentation/MacOSX/Conceptual/BPFrameworks/Tasks/IncludingFrameworks.html
[2] https://guides.cocoapods.org/syntax/podspec.html#module_name

[fkgozali]

Btw, it looks like we hit the same problem back in the day: CocoaPods/CocoaPods#4605 (comment)

And the suggested workaround for now is to add HEADER_SEARCH_PATH, e.g. https://github.com/facebook/react-native/pull/12089/files#diff-66230b3e029caa37b0fbdc8cbd47f4abR50

[jtreanor]

Thanks for taking a look at this @fkgozali. What you have summarised here matches my understanding of the situation.

Problem 1: Colliding .framework names

• It may be possible to trick all these .framework files to look for more header search paths, but I don't think that's right

Yeah. Although this might be possible, I haven't found a way to do this. Even if it was, I'm not sure it would be a good idea.

Given that we have a lot of #import <React/*.h> in the codebase, it is best to preserve this namespace as much as possible. That means, all headers from React-Core.podspec should be preserved with React prefix to minimize breakages.

For the rest of the .podspec's, I think we'll have to adapt the new namespace (e.g. like what Fabric did). This could be breaking change, but I'm hoping it's minimized since it's not the React-Core lib.

This is what I have done in this PR. React/* namespace is preserved for all React-Core.podspec headers. Anecdotally this has been enough to avoid changes in most of the third-party libraries we are using in our project.

If we fully commit to the new namespacing, a few updates will need to be made to React.xcodeproj and the related projects to copy headers into the correct namespace. This would allow us to remove the has_include statements on this PR.

Problem 2: Incomplete Dependencies in the podspec

Some of our .podspecs are missing direct dependencies. E.g. React-Core is missing deps to React-jsi and glog. This should be fixed regardless, and should be safe to do so. Parts of this current PR should be split into a separate PRs to fix these deps.

That's a good point. I can make a new PR that makes these changes. There are also a few instances of missing HEADER_SEARCH_PATHS that should be safe to do separately.

Problem 3: Something broke this in v0.60
Based on #25349, it seems like something broke this in v0.60. I'm not sure which commits caused it, but it would be best for 0.60 dot release (?) to revert the offending commits. @jtreanor do you know?

Then, we can fix things forward in master.

Both Problem 1 and Problem 2 here were introduced by React.podspec being split into multiple podspecs for 0.60.0 (#23559 and others). Since everything was a subspec before this, it always produced one React.framework and there were no conflicts (Problem 1). The issues in Problem 2 were also introduced here and not caught because they don't show up when the podspecs are built as static libraries.

I don't think reverting the podspec split is a viable option here.

Btw, it looks like we hit the same problem back in the day: CocoaPods/CocoaPods#4605 (comment)

And the suggested workaround for now is to add HEADER_SEARCH_PATH, e.g. https://github.com/facebook/react-native/pull/12089/files#diff-66230b3e029caa37b0fbdc8cbd47f4abR50

Thanks for sharing! I don't think HEADER_SEARCH_PATHS allows us keep all existing <React/*> imports working, but I may be missing something here.

[kelset]

cc'ing in @fson since he was the author of the split

[jtreanor]

As suggested above, I have opened a new PR with the non-breaking changes from this one in #25496. These should address the issues in "Problem 2" mentioned in #25393 (comment).

Proposal for avoiding colliding .framework names

As for "Problem 1", I have come up with a way that we could make these changes without breaking any of the exiting <React/*> imports. I'm unsure if it is something we would want to do, but I'm posting it here for consideration.

1. Move React-Core.podspec to the root of the repo so that it can reference code in Libraries (../ doesn't work). Alternatively, a new podspec could be used.
2. Update React-Core to have a subspec for each library podspec that only includes the headers for the podspec.
3. Update each library podspec to only include the implementation source files.

I have tested this and it works. Here what it would look like for React-RCTImage and React-RCTNetwork.

React-Core.podspec

Pod::Spec.new do |s|
  s.name                   = "React-Core"
  s.version                = version
  s.summary                = "The core of React Native."
  s.homepage               = "http://facebook.github.io/react-native/"
  s.license                = package["license"]
  s.author                 = "Facebook, Inc. and its affiliates"
  s.platforms              = { :ios => "9.0", :tvos => "9.2" }
  s.source                 = source
-  s.source_files           = "**/*.{c,h,m,mm,S,cpp}"
-  s.exclude_files          = "DevSupport/**/*",
-                             "Fabric/**/*",
-                             "Inspector/**/*"
-  s.ios.exclude_files      = "**/RCTTV*.*"
-  s.tvos.exclude_files     = "Modules/RCTClipboard*",
-                             "Views/RCTDatePicker*",
-                             "Views/RCTPicker*",
-                             "Views/RCTRefreshControl*",
-                             "Views/RCTSlider*",
-                             "Views/RCTSwitch*"
-  s.private_header_files   = "Cxx*/*.h"
  s.compiler_flags         = folly_compiler_flags + ' ' + boost_compiler_flags
  s.header_dir             = "React"
  s.framework              = "JavaScriptCore"
  s.library                = "stdc++"
  s.pod_target_xcconfig    = { "HEADER_SEARCH_PATHS" => "\"$(PODS_TARGET_SRCROOT)/ReactCommon\" \"$(PODS_ROOT)/boost-for-react-native\" \"$(PODS_ROOT)/DoubleConversion\" \"$(PODS_ROOT)/Folly\"" }
  s.default_subspec         = "Default"

  s.subspec "Default" do |ss|
-    # no-op
+    ss.source_files           = "React/**/*.{c,h,m,mm,S,cpp}"
+    ss.exclude_files          = "React/DevSupport/**/*",
+                                "React/Fabric/**/*",
+                                "React/Inspector/**/*"
+    ss.ios.exclude_files      = "React/**/RCTTV*.*"
+    ss.tvos.exclude_files     = "React/Modules/RCTClipboard*",
+                                "React/Views/RCTDatePicker*",
+                                "React/Views/RCTPicker*",
+                                "React/Views/RCTRefreshControl*",
+                                "React/Views/RCTSlider*",
+                                "React/Views/RCTSwitch*"
+    ss.private_header_files   = "React/Cxx*/*.h"
  end

  s.subspec "CxxBridge" do |ss|
    # Make the C++ headers visible if they are needed
    ss.public_header_files   = "**/*.{h}"
+    ss.dependency              "React-Core/Default"
  end

+  s.subspec "RCTImage" do |ss|
+    ss.public_header_files = "Libraries/Image/*.{h}"
+    ss.source_files        = "Libraries/Image/*.{h}"
+    ss.dependency            "React-Core/Default"
+  end
+
+  s.subspec "RCTNetwork" do |ss|
+    ss.public_header_files = "Libraries/Network/*.{h}"
+    ss.source_files        = "Libraries/Network/*.{h}"
+    ss.dependency            "React-Core/Default"
+  end

  s.dependency "Folly", folly_version
  s.dependency "React-cxxreact", version
  s.dependency "React-jsi", version
  s.dependency "React-jsiexecutor", version
  s.dependency "yoga", "#{version}.React"
  s.dependency "glog"
end

React-RCTImage.podspec

Pod::Spec.new do |s|
  s.name                   = "React-RCTImage"
  s.version                = version
  s.summary                = "A React component for displaying different types of images."
  s.homepage               = "http://facebook.github.io/react-native/"
  s.documentation_url      = "https://facebook.github.io/react-native/docs/image"
  s.license                = package["license"]
  s.author                 = "Facebook, Inc. and its affiliates"
  s.platforms              = { :ios => "9.0", :tvos => "9.2" }
  s.source                 = source
- s.source_files           = "*.{h,m}"
+ s.source_files           = "*.{m}"
  s.preserve_paths         = "package.json", "LICENSE", "LICENSE-docs"
  s.header_dir             = "RCTImage"

  s.dependency "React-Core", version
+ s.dependency "React-Core/RCTImage", version
  s.dependency "React-RCTNetwork", version
end

React-RCTImage.podspec

Pod::Spec.new do |s|
  s.name                   = "React-RCTNetwork"
  s.version                = version
  s.summary                = "The networking library of React Native."
  s.homepage               = "http://facebook.github.io/react-native/"
  s.license                = package["license"]
  s.author                 = "Facebook, Inc. and its affiliates"
  s.platforms              = { :ios => "9.0", :tvos => "9.2" }
  s.source                 = source
-  s.source_files           = "*.{h,m,mm}"
+  s.source_files           = "*.{m,mm}"
  s.preserve_paths         = "package.json", "LICENSE", "LICENSE-docs"
  s.header_dir             = "RCTNetwork"

  s.dependency "React-Core", version
+  s.dependency "React-Core/RCTNetwork", version
end

With this approach, there is a single React.framework with has all the headers for React-Core and the libraries that are used. RCTImage.framework and RCTNetwork.framework have no headers and their internal imports should now use the form <React/*>.

What do you think? Is this a crazy idea or could it be the compromise that we need?

[fkgozali]

Proposal for avoiding colliding .framework names

@jtreanor Thanks for the proposal. I think it will be worth trying it out. A few things we can clean up:

• There is React.podspec and there is React-Core.podspec, perhaps we should just merge the 2 and leave them at the root dir?
• The subspec approach seems like an ok approach to solve this short term (I'm not sure how well that scales long term as we add more targets, but we'll deal with it when it happens).
• We added a bunch of new .podspec files recently (see commit stack of fde8a4c), and those can be made subspec as needed

[jtreanor]

Great, thanks @fkgozali.

I will go ahead with trying this approach. I should be able to get to it in the next day or two.

[jtreanor]

Okay, I have put together a draft PR for the subspec solution in #25619. It was quite a bit more involved than I anticipated and there is still some work to do. I'm keen to get some feedback before spending any more time on it.

It doesn't avoid all breaking changes but <React/*> imports will continue to work. The tradeoff is a more complex implementation to maintain.

[kesha-antonov]

Nice job!
Waiting for this

[jtreanor]

Closing in favour of #25619.