[RCTImageLoader] Make C++ requirement opt-in

[alloy]

Summary

When building as a framework these headers get automatically added to the framework umbrella header for React-Core. Instead of converting all the React sources to ObjC++ files and still forcing external users that build native source (and link against a framework build) to also compile as ObjC++, this makes the attribution related methods that were added in fdcdca4 opt-in to ObjC++ builds.

This is also the reason for the current failure of the CI test_ios_frameworks run.

Changelog

I’m unsure if this change really warrants an entry in the CHANGELOG, as it’s more of an amendment of the (afaik) unreleased change.

[iOS] [Fixed] - Make framework builds work again by making RCTImageLoader C++ requirement opt-in

Test Plan

I tested static and dynamic (framework) builds and ran the test suite.

This change should make the test_ios_frameworks CI run build again, but it may still fail overall as in my local testing one of the tests leads to a segfault (which I will try to address separately).

[alloy]

A back ported version for the 0.62 branch is here #27731

[alloy]

but it may still fail overall as in my local testing one of the tests leads to a segfault (which I will try to address separately)

Looks like this is only a problem when running on iOS 13, CI runs iOS 12.4. Nonetheless a legit segfault, for when somebody on iOS 13 would try to serialise strings that can’t be utf8 encoded. Will do a bit of time-boxed sleuthing.

[facebook-github-bot]

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

[fkgozali]

Hmm, I don't think I intended RCTImageURLLoaderWithAttribution.h and RCTImageURLLoaderWithAttribution.h to be exported outside of the RCTImage lib/spec, so perhaps we can blacklist these 2 files in the podspec exported headers?

Maybe this import should do local header import instead:

react-native/Libraries/Image/RCTImageLoaderWithAttributionProtocol.h

Line 11 in eb95b2f

#import <React/RCTImageURLLoaderWithAttribution.h>

[alloy]

Ah, I see. That should work, I’ll update it next week 👍

[alloy]

@fkgozali I took a stab at making these headers private, but found that–while it works well for framework builds–it will fail a static build.

The reason is that e.g. RCTImageView.mm imports React/RCTImageLoaderWithAttributionProtocol.h rather than RCTImage/RCTImageLoaderWithAttributionProtocol.h and thus the header is always expected to be exported by the React namespace. I think that the current version of my PR is the simpler solution to this issue, what do you think?

[fkgozali]

The reason is that e.g. RCTImageView.mm imports React/RCTImageLoaderWithAttributionProtocol.h rather than RCTImage/RCTImageLoaderWithAttributionProtocol.h and thus the header is always expected to be exported by the React namespace.

I see the complication now. RCTImageView.mm is pre-Fabric, so I'm hoping with Fabric rolled out, this gets easier to solve (because Fabric is ObjC++ everywhere). It seems like your current change is the most practical to unblock RC atm. I'll merge.

[react-native-bot]

This pull request was successfully merged by @alloy in 25571ec.

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