Header changes required for 0.40 by christopherdro · Pull Request #670 · microsoft/react-native-code-push

christopherdro · 2017-01-12T23:14:41Z

These changes were required for me to properly build the project on the latest version of react native.
Related #665 #638

msftclas · 2017-01-12T23:14:45Z

Hi @christopherdro, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

axemclion · 2017-01-13T02:09:09Z

@christopherdro Thanks for the PR. Would this be a duplicate of #665 ?

christopherdro · 2017-01-13T02:45:58Z

@axemclion Are the changes in #665 suppose to achieve the same thing?

I had to cherry pick the 2 open PR's plus these changes to get our app to build properly.
Although I haven't tested it. Will likely push to stage tomorrow and can confirm back here afterwards.

Here is how our forked version currently looks.
https://github.com/wildlifela/react-native-code-push/tree/rn-40

msftgits · 2017-01-13T16:25:50Z

Hi, I am closing and re-opening this PR to bump the CLA bot. Sorry for the inconvenience!

dingbat · 2017-01-18T00:34:27Z

Any updates on this? This PR alone fixed errors for me on RN 0.40.0 (without needing the other things in the rn-40 branch of @christopherdro's fork.)

bitcoinvsalts · 2017-01-18T02:38:42Z

+1

sergey-akhalkov · 2017-01-19T14:27:14Z

@@ -1,5 +1,5 @@
 #if __has_include("RCTEventEmitter.h")
-#import "RCTEventEmitter.h"
+#import <React/RCTEventEmitter.h>


To make CodePush able to work both with react-native@0.40.0 and react-native@0.39.2 we probably should use:

#if __has_include("RCTEventEmitter.h") #import "RCTEventEmitter.h" #elif __has_include(<React/RCTEventEmitter.h>) #import <React/RCTEventEmitter.h> #else #import "React/RCTEventEmitter.h" // Required when used as a Pod in a Swift project #endif

sergey-akhalkov · 2017-01-19T14:28:05Z

+#import <React/RCTConvert.h>
+#import <React/RCTEventDispatcher.h>
+#import <React/RCTRootView.h>
+#import <React/RCTUtils.h>


The same here:

#if __has_include("RCTAssert.h") #import "RCTAssert.h" #else #import <React/RCTAssert.h> #endif #if __has_include("RCTBridgeModule.h") #import "RCTBridgeModule.h" #else #import <React/RCTBridgeModule.h> #endif #if __has_include("RCTConvert.h") #import "RCTConvert.h" #else #import <React/RCTConvert.h> #endif #if __has_include("RCTEventDispatcher.h") #import "RCTEventDispatcher.h" #else #import <React/RCTEventDispatcher.h> #endif #if __has_include("RCTRootView.h") #import "RCTRootView.h" #else #import <React/RCTRootView.h> #endif #if __has_include("RCTUtils.h") #import "RCTUtils.h" #else #import <React/RCTUtils.h> #endif

sergey-akhalkov · 2017-01-19T14:28:50Z

@@ -1,5 +1,5 @@
 #import "CodePush.h"
-#import "RCTConvert.h"
+#import <React/RCTConvert.h>


#import "CodePush.h" #if __has_include("RCTConvert.h") #import "RCTConvert.h" #else #import <React/RCTConvert.h> #endif

sergey-akhalkov · 2017-01-19T14:29:00Z

@@ -1,5 +1,5 @@
 #import "CodePush.h"
-#import "RCTConvert.h"
+#import <React/RCTConvert.h>


#import "CodePush.h" #if __has_include("RCTConvert.h") #import "RCTConvert.h" #else #import <React/RCTConvert.h> #endif

sergey-akhalkov · 2017-01-19T14:35:20Z

@dingbat - hi. I've tried to build initial React Native project (execute react-native init myapp command) using this PR and got build failed - CodePush.h file not found, it means that #665 and #638 PRs is steel required for react-native@0.40.0.

By the way I think it may be a good idea to apply this PR in next release of CodePush due to breaking changes in https://github.com/facebook/react-native/releases/tag/v0.40.0 . But this PR has no back compatibility with React Native having version v0.39.2. To make CodePush able to work both with react-native@0.40.0 and react-native@0.39.2 header files should use __has_include(...) statements. (I've added comments in files changes of this PR).

@christopherdro, could you please rebase your commit to provide back compatibility? Please let me know if you see any issues or have any questions.

AndrewJack · 2017-01-19T17:14:45Z

@sergey-akhalkov Why don't you bump the major version for 0.40 and later, and continue to support 0.29 and older on the 1.x version in a separate 1.x branch?

Just like how bugsnag have done it - https://github.com/bugsnag/bugsnag-react-native/releases

richardhuaaa · 2017-01-19T19:22:12Z

@AndrewJack - I think the main reason is that we'd like to avoid the overhead of maintaining multiple active branches simultaneously and porting features/fixes between them etc. Although I'd be interested to hear any alternative arguments.

ivpusic · 2017-01-19T23:16:37Z

are you planning to integrate these changes soon? This is the only thing blocking me from migrating to RN 0.40.

richardhuaaa · 2017-01-20T01:00:52Z

@buptkang and I are actively looking at it, we expect to have it released tomorrow or on Monday.

richardhuaaa · 2017-01-24T03:12:51Z

Hi all, we ran into some minor unanticipated issues and have a supplemental PR up with some tweaks to ensure that we are backwards compatible and that some other header issues are taken care of. It will be merged tonight, and we will make a release tomorrow morning!

buptkang · 2017-01-24T20:17:37Z

@christopherdro @dingbat @jsappme @AndrewJack @ivpusic @nihgwu @martijndeh:
Apologies for the delay - we've merged all of the required fixes and verified everything. react-native-code-push@1.17.0-beta has been published to npm with support for react-native@0.40.0. Thanks for your patience, and release notes are coming soon!

ivpusic · 2017-01-24T20:43:22Z

awesome! thanks a lot for your time and effort!

wkw · 2017-02-07T22:33:21Z

In case this helps anyone else upgrading iOS, I was getting build failures for a lot of duplicate interface definitions after upgrading to rncp 1.17.0-beta. They all came from this line in my AppDelegate.m: #import "CodePush.h". I guess that got left when I r-n unlinked then after re-linking, my appdelegate looked like:

#import "AppDelegate.h"
#import <CodePush/CodePush.h>
#import "CodePush.h"

Removing #import "CodePush.h" made the build... well, build!

I have yet to ever test code pushing, so I can't say whether there are any other issues.

ghost · 2017-02-08T01:20:42Z

If it's a breaking change may I suggest bumping the major in accordance with SemVer unless you've got something more practical going on.

axemclion · 2017-02-08T01:35:43Z

We have been using (this line](https://github.com/Microsoft/react-native-code-push/blob/e42485243d9466fb6c00937bd9f67cee0d96de53/ios/CodePush/RCTConvert%2BCodePushInstallMode.m#L3 for backward compat, so I think we dont have to bump up the major version.

#if __has_include(<React/RCTConvert.h>)
#import <React/RCTConvert.h>
#else
#import "RCTConvert.h"
#endif

simonmitchell · 2017-02-09T10:30:25Z

All of the above __has_includes need to be the other way around apart from @axemclion's example, because technically #import "RCTConvert.h" is still available to the compiler, it just causes compiler warnings due to a duplicate import!

#if __has_include(<React/RCTRedBox.h>)
#import <React/RCTRedBox.h>
#elif __has_include("RCTRedBox.h")
#import "RCTRedBox.h"
#elif __has_include("React/RCTRedBox.h")
#import "React/RCTRedBox.h"   // Required when used as a Pod in a Swift project
#endif

axemclion · 2017-02-09T15:58:21Z

cc @BretJohnson - what do you think of the comment above ?

simonmitchell · 2017-02-09T16:01:48Z

I added this style of import into a pull-request I was merging into another library: react-native-navigation, I only ran the project with RN 0.40.0, so this way around may cause issues with RN < 0.40.0, but the examples above mine certainly didn't work with RN 0.40.0 :)

Update header imports for RN 0.40

693c177

msftclas added the cla-required label Jan 12, 2017

msftgits closed this Jan 13, 2017

msftgits reopened this Jan 13, 2017

msftclas added the cla-signed label Jan 13, 2017

msftgits removed the cla-required label Jan 13, 2017

sergey-akhalkov reviewed Jan 19, 2017

View reviewed changes

This was referenced Jan 20, 2017

Android app crash when using installMode IMMEDIATE, and ON_NEXT_RESTART working as expected. #676

Closed

ios codepush error CodePushDownloadProgress #516

Closed

buptkang merged commit 25851d2 into microsoft:master Jan 24, 2017

buptkang mentioned this pull request Jan 24, 2017

Upgrade RN 0.40 Finalization #678

Merged

This was referenced Jan 26, 2017

Support RN 0.39.2 and 0.40.0 react-native-share/react-native-share#102

Merged

Support RN 0.39.2 and RN 0.40.0 TheWidlarzGroup/react-native-video#469

Closed

messense mentioned this pull request Feb 7, 2017

Work in react-native 0.41 wix/react-native-navigation#723

Closed

messense mentioned this pull request Feb 15, 2017

Support RN 0.39 and RN 0.40+ jpush/jpush-react-native#148

Merged

dhorelik mentioned this pull request Feb 16, 2017

invert __has_includes checks adjust/react_native_sdk#7

Closed

Kudo mentioned this pull request Dec 20, 2021

Upgrading to expo@44.0.0 breaks iOS build of amazon-cognito-identity-js expo/expo#15622

Closed

Conversation

christopherdro commented Jan 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

msftclas commented Jan 12, 2017

Uh oh!

axemclion commented Jan 13, 2017

Uh oh!

christopherdro commented Jan 13, 2017

Uh oh!

msftgits commented Jan 13, 2017

Uh oh!

dingbat commented Jan 18, 2017

Uh oh!

bitcoinvsalts commented Jan 18, 2017

Uh oh!

sergey-akhalkov Jan 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sergey-akhalkov Jan 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sergey-akhalkov Jan 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sergey-akhalkov Jan 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sergey-akhalkov commented Jan 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AndrewJack commented Jan 19, 2017

Uh oh!

richardhuaaa commented Jan 19, 2017

Uh oh!

ivpusic commented Jan 19, 2017

Uh oh!

richardhuaaa commented Jan 20, 2017

Uh oh!

richardhuaaa commented Jan 24, 2017

Uh oh!

buptkang commented Jan 24, 2017

Uh oh!

ivpusic commented Jan 24, 2017

Uh oh!

wkw commented Feb 7, 2017

Uh oh!

ghost commented Feb 8, 2017 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

axemclion commented Feb 8, 2017

Uh oh!

simonmitchell commented Feb 9, 2017

Uh oh!

axemclion commented Feb 9, 2017

Uh oh!

simonmitchell commented Feb 9, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

christopherdro commented Jan 12, 2017 •

edited

Loading

sergey-akhalkov Jan 19, 2017 •

edited

Loading

sergey-akhalkov Jan 19, 2017 •

edited

Loading

sergey-akhalkov Jan 19, 2017 •

edited

Loading

sergey-akhalkov Jan 19, 2017 •

edited

Loading

sergey-akhalkov commented Jan 19, 2017 •

edited

Loading

ghost commented Feb 8, 2017 •

edited by ghost

Loading