Fix <React/... BC break: Add ifndef surrounds to iOS headers

[robhogan]

Motivation

Since e1577df, and even after 59407f3, changing header search paths and switching to #import <React/Thing.h> within RN means that user and third-party code has to be changed to match. As far as I understand it, that means a new BC (major) release will be required of every third party library which imports iOS headers. App devs will need to wait until third-party libs are fixed or rely on patches. Painful!

Proposal

App and library developers are required to change because, when both #import <React/RCTFoo.h> and #import "RCTFoo.h" appear, we get redefinition issues. If we surround the headers in #ifndef RCTFOO_H #define RCTFOO_H ... #endif that's not an issue, and users are free to import headers by a mix of styles.

Testing

As per motivation, this build against current master fails because the header change breaks compatibility with these two third party libs, things like error: redefinition of 'RCTLogLevel':

yarn cache clean
react-native init fooproj --version react-native@facebook/react-native#bc285de79975f7990d5edb365a6ef76fa337c7e1
cd fooproj
yarn add react-native-vector-icons react-native-orientation
react-native link
react-native run-ios

This PR builds fine:

yarn cache clean
react-native init barproj --version react-native@rh389/react-native#ifndef
cd barproj
yarn add react-native-vector-icons react-native-orientation
react-native link
react-native run-ios

I'm fairly certain it's not possible for this PR to do harm, but it'd be interesting to know whether it helps those using different configurations (Swift, Pods etc).

Edit: FYI, I ran find React Libraries -name *.h -execdir ~/addifndef.sh {} \; with https://gist.github.com/rh389/cb882af964936ff9eb8b976620592ec1 and followed up with some manual fixing for files without the standard 10 line boilerplate header.

[chirag04]

cc @javache @janicduplessis

[robhogan]

cc @bestander - making sure you're aware of the iOS BC currently in 0.40.

[bestander]

I'll leave this to IOS experts.

[janicduplessis]

I'm not a big fan of the added complexity but I'll leave it up to @javache to decide what we should do here.

[robhogan]

I agree this is a little messy, but if RN is committed to making this change to the way headers are imported I don't see any other way to avoid breaking compatibility with all third-party native modules/libs, which IMO is a huge BC. Any alternatives?

[K-Leon]

Breaking compatibility with all third-party modules without a proper deprecation period is a hard step. Remembering the trouble the slow adoption of direct react import caused. Taking into consideration that this bc is for the casual dev even harder to fix then just editing the js libs.. this will get tough considering the amount of issues that boil down to proper ios dependency linking.

So the solution from @rh389 is the best thing i've seen to prevent this.

[nihgwu]

I think we can provide a way to automatically update the import headers path and HEADER_SEARCH_PATHS via react-native link or react-native upgrade

[robhogan]

@nihgwu - That'd be great but I haven't been able to find an incantation using HEADER_SEARCH_PATHS that builds without either applying this PR or by altering the #import statements within third-party packages (ie, underneath node_modules).

If you're suggesting patching imports inside third-party code on the fly, it'd need to be in a way that survives reinstalling node_modules - so you'd need a postinstall script or an xcode build phase. That seems uglier than this PR though, to be honest.

[javache]

I'm not a big fan of the added complexity, and it also worries that me this would leave it totally optional for 3rd party developers to upgrade their imports, or not, further extending the support period. Internally at FB we've moved everything to <React/...> so this style of header would likely break whenever a new header is added to open-source.

Would it be possible to handle this correctly by setting the 3rd party's project' HEADER_SEARCH_PATHS to PRODUCT_DIR/include/**, where the new headers are currently located.

[robhogan]

I've tried a few variations using $(BUILT_PRODUCTS_DIR)/include without success unfortunately.

I'd argue that leaving things optional with a support period (allowing for reasonable deprecation) is better than making the change mandatory with absolutely no overlap period at all. The 0.39 docs don't even mention that a change is expected.

A big part of the problem is that any library which makes this change will immediately break their support for RN <=0.39 (unless there's an option I'm not aware of?), so most will be forced to maintain two branches, and none that I'm aware of support RN 0.40 yet in a released version.

Taking an obvious, facebook-maintained example, do you know what the plan is for https://github.com/facebook/react-native-fbsdk? I note there's a PR but some confusion, and no released support for 0.40 yet.

My concern is that very few app developers will be able to adopt RN 0.40 at all - we'll probably be a release or two further down the line before there's enough third-party support to upgrade and anyone stuck on RN <=0.39 will be left in the cold by the community. IMO that justifies a bit of additional, temporary complexity.

Anyway, I've made my argument - your call ;)

[messense]

Please do not make a breaking change without deprecation first.

…

在 2017年1月4日，23:41，Rob Hogan ***@***.***> 写道： I've tried a few variations using $(BUILT_PRODUCTS_DIR)/include without success unfortunately. I'd argue that leaving things optional with a support period (allowing for reasonable deprecation) is better than making the change mandatory with absolutely no overlap period at all. The 0.39 docs don't even mention that a change is expected. A big part of the problem is that any library which makes this change will immediately break their support for RN <=0.39 (unless there's an option I'm not aware of?), so most will be forced to maintain two branches, and none that I'm aware of support RN 0.40 yet in a released version. Taking an obvious, facebook-maintained example, do you know what the plan is for https://github.com/facebook/react-native-fbsdk? I note there's a PR but some confusion, and no released support for 0.40 yet. My concern is that very few app developers will be able to adopt RN 0.40 at all - we'll probably be a release or two further down the line before there's enough third-party support to upgrade and anyone stuck on RN <=0.39 will be left in the cold by the community. IMO that justifies a bit of additional, temporary complexity. Anyway, I've made my argument - your call ;) — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

[ptomasroos]

Personally no one is requiring anyone to track 0.40. I can't see any reason why a deprecation period would aid here. It would most likely just delay the community to move to the new headers. I think this should just be closed and accept the fact that breaking changes in projects that are you and has not started to version against 1.*

[robhogan]

App developers aren't required to track 0.40 but the library community is. I think the problem is illustrated by the fact that right now 0.40 is the latest stable release, and has practically no community native package support. Any new users trying RN for the first time today, who try to use common, popular libraries, including those maintained by facebook itself, will be met with cryptic build errors.

Not ideal, surely.

[chirag04]

FYI, i was able to get the failing test case from @rh389 to work fine in both dev and release mode after following the migration guide here: https://github.com/facebook/react-native/releases/tag/v0.40.0.

[ericvicenti]

Yeah I agree with @javache that the added complexity isn't worth it for a change that needs to be made anyways. We will try to keep breaking changes to a minimum, but sometimes they happen.