[iOS] CocoaPods frameworks compatibility: Step 2

[jtreanor]

Summary

This is my proposal for fixing use_frameworks! compatibility without breaking all <React/*> imports I outlined in #25393 (comment). If accepted, it will fix #25349.

It builds on the changes I made in #25496 by ensuring each podspec has a unique value for header_dir so that framework imports do not conflict. Every podspec which should be included in the <React/*> namespace now includes it's headers from React-Core.podspec.

The following pods can still be imported with <React/*> and so should not have breaking changes: React-ART,React-DevSupport, React-CoreModules, React-RCTActionSheet, React-RCTAnimation, React-RCTBlob, React-RCTImage, React-RCTLinking, React-RCTNetwork, React-RCTPushNotification, React-RCTSettings, React-RCTText, React-RCTSettings, React-RCTVibration, React-RCTWebSocket .

There are still a few breaking changes which I hope will be acceptable:

• React-Core.podspec has been moved to the root of the project. Any Podfile that references it will need to update the path.
• React-turbomodule-core's headers now live under <turbomodule/*> Replaced by [iOS] CocoaPods frameworks compatibility: Step 2 #25619 (comment).
• React-turbomodulesamples's headers now live under <turbomodulesamples/*> Replaced by [iOS] CocoaPods frameworks compatibility: Step 2 #25619 (comment).
• React-TypeSaferty's headers now live under <TypeSafety/*> Replaced by [iOS] CocoaPods frameworks compatibility: Step 2 #25619 (comment).
• React-jscallinvoker's headers now live under <jscallinvoker/*> Replaced by [iOS] CocoaPods frameworks compatibility: Step 2 #25619 (comment).
• Each podspec now uses s.static_framework = true. This means that a minimum of CocoaPods 1.5 (released in April 2018) is now required. This is needed so that the __has_include conditions can still work when frameworks are enabled.

Still to do:

• Including React-turbomodule-core with use_frameworks! enabled causes the C++ import failures we saw in [iOS] RN 0.60.0-rc.2 fails to build with CocoaPods frameworks #25349. I'm sure it will be possible to fix this but I need to dig deeper (perhaps a custom modulemap would be needed). Addressed by 3357351.
• I haven't got Fabric working yet. I wonder if it would be acceptable to move Fabric out of the <React/*> namespace since it is new? 🤔

Changelog

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

Test Plan

Sample Project

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 [email protected]:jtreanor/react-native-cocoapods-frameworks.git
2. git checkout fix-frameworks-subspecs
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.

RNTesterPods

RNTesterPodsPods can now work with or without use_frameworks!.

1. Go to the RNTester directory and run pod install.
2. Run the tests in RNTesterPods.xcworkspace to see that everything still works fine.
3. Uncomment the use_frameworks! line at the top of RNTester/Podfile and run pod install again.
4. Run the tests again and see that it still works with frameworks enabled.

[fkgozali]

There are still a few breaking changes which I hope will be acceptable

The ones you listed are fine to be changed. I had internal diff renaming typesafety stuff to RCTTypeSafety too, so we're on the same page. For TM, I was thinking to create a higher level TM spec with core/sample subspec, etc (like fabric) but we can deal with it later. I think a few of them under ReactCommon can be combined into a common podspec, with subspec for each.

For fabric, most of fabric is already not using <React/, you meant just the ObjC part (RCTFabric)? If so, I think it's ok to move it out. I suggest RCTFabric namespace for the time being.

[fkgozali]

React-turbomodule-core's headers now live under <turbomodule/>
React-turbomodulesamples's headers now live under <turbomodulesamples/>
React-TypeSaferty's headers now live under <TypeSafety/>
React-jscallinvoker's headers now live under <jscallinvoker/>

You may want to make these separate PRs for easier merging

[fkgozali]

@ericlewis - thoughts?

[fkgozali]

@jtreanor - 0c8c95f handled the TypeSafety stuff

I'm experimenting with ReactCommon podspec as I mentioned, so hopefully that makes it easier for you as well

[fkgozali]

Notes:

I tested header prefix like <react/turbomodule/*.h> - this worked fine if not using frameworks, but Xcode failed to find the header when use_frameworks! is set. I don't know what the workaround is at this point, so to unblock, I'll just flatten jscallinvoker and turbomodule into <ReactCommon/ for the time being. All of these are just subspecs under the new ReactCommon.podspec that I'll be adding.

[fkgozali]

@jtreanor I landed a few commits to deal with jsireact stuffs:

• 6e7ce9c
• 394c9a5
• c1b0f39
• 3b6f6ca

use_framework! still hasn't fully worked since we need the rest of your fix, but TM seems to build correctly. I'm not sure what C++ error you got, so probably good to try it out again after rebasing.

Let me know if I can help further here.

[jtreanor]

Thanks a lot @fkgozali!

These changes will be very helpful.

[jtreanor]

Okay, I have rebased master and @fkgozali's changes have helped a lot. Thank you again!

I am still seeing the C++ error I mentioned above, so I'll give more detail here. When I add use_frameworks! to RNTester/Podfile, I get this build error:

As you can see, this is the same error I encountered when investigating #25349 that led me to make C++ headers private by default. The problem seems to be that an Objective C file importing the React module, includes all public headers and fails to build the C++. I don't understand why the fix I made previously is not working.

As a hack and to confirm this is the only issue, I was able to get the build working by updating Pods/Target Support Files/React-Core.common/React-Core.common-umbrella.h as follows:

...
+#ifdef __cplusplus
#import "JSCExecutorFactory.h"
#import "NSDataBigString.h"
#import "RCTCxxBridgeDelegate.h"
#import "RCTMessageThread.h"
#import "RCTObjcExecutor.h"
#import "DispatchMessageQueueThread.h"
#import "RCTCxxMethod.h"
#import "RCTCxxModule.h"
#import "RCTCxxUtils.h"
#import "RCTNativeModule.h"
#import "RCTFollyConvert.h"
+#endif
...

This stops the C++ headers from being imported into the umbrella header and the build succeeds. Of course, modifying this file doesn't solve our problem, but confirms it.

The only way to customise the umbrella header is to define a custom modulemap and umbrella header. However, I feel like there should be a way to solve the problem without resorting to this.

I will keep investigating but I'm open to any ideas at all.

[fkgozali]

I am still seeing the C++ error I mentioned above

Hmm, perhaps there's a missing compiler_flag in the individual podspec/subspec to hint that it's C++ code? React-RCTWebSocket.podspec doesn't seem to have any C++ flag configured, so maybe if you look at Xcode workspace, that framework is compiled for ObjC?

[jtreanor]

Hmm, perhaps there's a missing compiler_flag in the individual podspec/subspec to hint that it's C++ code? React-RCTWebSocket.podspec doesn't seem to have any C++ flag configured, so maybe if you look at Xcode workspace, that framework is compiled for ObjC?

Unfortunately, I don't think that would suffice. If we fix the problem for React-RCTWebSocket and the other RN pods, any user code that imports a React header into an ObjC file would still get this error.

[fkgozali]

any user code that imports a React header into an ObjC file would still get this error.

I think it's fine to require that callsites have .mm extension (ObjC++), not .m (ObjC), given that down the road everything will involve C++ (TurboModule/Fabric). If that's the only concern, I think it's ok to proceed (assuming we fix all podspecs to be C++ compatible)

[jtreanor]

Oh, I see. I'll look a little more to see if there is a simple way to avoid that requirement but otherwise I'll proceed as you suggest.

I am mostly wondering why this wasn't an issue prior to the podspec split. I'd like to understand what's going on here before choosing the best solution.

[fkgozali]

Another approach (not always applicable) is to move the .h imports to the .mm instead of inside another .h. That would isolate the change to just the relevant .mm files, so other libs aren't affected.

[pull-bot]

	Warnings
⚠️	🔒 package.json - Changes were made to package.json. This will require a manual import by a Facebook employee.

Generated by 🚫 dangerJS against e2a73e8

[fkgozali]

As a result of this, use_frameworks! now works with RNTesterPods. I have also made a few small updates to get the tests working and updated the PR description.

@jtreanor Thank you so much for working on this! I didn't have a chance to review/merge this yet due to unrelated work. cc @hramos @cpojer. I'll get back to this some time next week if others don't get to merge it by then.

The only thing not working at this point is Fabric. I'm not sure if we should address that in this PR or separately.

Yeah, this is fine. Fabric had missing configuration unrelated to this use_frameworks! issue, so it's ok to solve it separately. It's also not enabled by default yet, so it won't block any release atm.

[kdawgwilk]

I appreciate all the work going into fixing this issue but my concern is how do we make sure this doesn’t break going forward? It would be a shame if we fixed this now but broke it in another release. Will having this integrated into RNTesterPods make sure that all of these different CocoaPods scenarios don’t break? e.g. use_frameworks! enabled or disabled

[fkgozali]

I appreciate all the work going into fixing this issue but my concern is how do we make sure this doesn’t break going forward?

There's now a CI test to cover standard pods build with RNTester, so that will help prevent future breakage. We'll likely need to add a second variant using use_framework! though - cc @hramos

[jtreanor]

Yes, we can absolutely add a CI step to integrate and test RNTesterPods with use_frameworks!. Of course, this will increase CI run time but I'm happy to add it if that's acceptable.

[fkgozali]

Yes, we can absolutely add a CI step to integrate and test RNTesterPods with use_frameworks!. Of course, this will increase CI run time but I'm happy to add it if that's acceptable.

We'll likely need a new RNTester project just for this variant. Let's visit this after we fully resolve this issue.

[jtreanor]

We'll likely need a new RNTester project just for this variant.

I don't think that will be needed. For example, a change like this to RNTester/Podfile would allow running USE_FRAMEWORKS=1 pod install to install with frameworks:

-  # Uncomment for Swift
-  #  use_frameworks!
+ use_frameworks! if ENV['USE_FRAMEWORKS'] == 1

Let's visit this after we fully resolve this issue.

Okay, I'll hold off on any changes for this to allow whats here to be reviewed first.

[fkgozali]

could you help me understand why this line matters?

[fkgozali]

OK, found the explanation in your summary:

Each podspec now uses s.static_framework = true. This means that a minimum of CocoaPods 1.5 (released in April 2018) is now required. This is needed so that the __has_include conditions can still work when frameworks are enabled.

I think it's worth exploring not using static_framework, but we can do it outside of this PR. I'm not sure if it's 100% possible given the current directory structure.

[jtreanor]

Just to clear this up completely, here is a more thorough explanation.

In a number of places throughout the codebase, there is code like this:

#if RCT_DEV && __has_include(<React/RCTDevLoadingView.h>)
    // Note: RCTDevLoadingView should have been loaded at this point, so no need to allow lazy loading.
    RCTDevLoadingView *loadingView = [weakSelf moduleForName:RCTBridgeModuleNameForClass([RCTDevLoadingView class])
                                       lazilyLoadIfNecessary:NO];
    [loadingView updateProgress:progressData];
#endif

The trouble is that this code lives in React-Core.podspec but RCTDevLoadingView is part of React-DevSupport. It works fine when not using frameworks but when use_frameworks! is used and RCT_DEV is enabled there is an error compiling React-Core.podspec as RCTDevLoadingView is not found. This is because React-DevSupport is not linked with React-Core.

The solution is to add s.static_framework = true to each of the podspecs. Now all symbols are linked into the same executable so RCTDevLoadingView is present at build time. It is necessary to add s.static_framework = true to all podspecs as static frameworks are not allowed to have dynamic dependencies.

As mentioned above the main disadvantage of this is that it requires CocoaPods 1.5 or later to be used (released in April 2018). Another drawback is that I believe CocoaPods complains if a target has transitive static dependencies. This means any third party podspec that depends on React would also need to have s.static_framework = true.

I think it's worth exploring not using static_framework, but we can do it outside of this PR. I'm not sure if it's 100% possible given the current directory structure.

The only way I can think of that would be possible would be to make React-DevSupport a subspec of React-Core as it was before the podspec split.

[fkgozali]

This means any third party podspec that depends on React would also need to have s.static_framework = true.

Yeah, this is my worry: it could be too limiting for those libs.

The only way I can think of that would be possible would be to make React-DevSupport a subspec of React-Core as it was before the podspec split.

I was actually trying this on top of your PR, but got a dependency cycle error, so I didn’t proceed. Perhaps it’s worth another look?

[jtreanor]

I was actually trying this on top of your PR, but got a dependency cycle error, so I didn’t proceed. Perhaps it’s worth another look?

I can take a look.

[jtreanor]

Ah, I remember the problem with that now. DevSupport depends on React-RCTWebSocket which depends on React-Core.

If we make React-RCTWebSocket a subspec too, it will work. I'll open a PR.

[jtreanor]

Okay, I've done that here: #25816

[facebook-github-bot]

@fkgozali has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[fkgozali]

I think this one would be a breaking change (more files will be moved here for TurboModule migration), I'll see if I can move it up as a subspec inside React-Core.podspec

[fkgozali]

nvm, I think this is meaningless now that you removed the .h files from the list

[jtreanor]

Yes, thats right.

[react-native-bot]

This pull request was successfully merged by @jtreanor in 8131b7b.

^{When will my fix make it into a release? | Upcoming Releases}

[jtreanor]

Thanks for reviewing @fkgozali!

[fkgozali]

Thanks for reviewing @fkgozali!

You’re welcome! Thanks for the all the work.

I think as the next steps we can:

• add framework variant to CI test for RNTester
• see if we can not use static_frameworks for better third party support?

[jtreanor]

I think as the next steps we can:

• add framework variant to CI test for RNTester

I've opened a draft PR for this here: #25818. It may require some discussion on the best approach.

• see if we can not use static_frameworks for better third party support?

PR opened for this here: #25816 as mentioned above.