Upgrade RN 0.40 Finalization #678
Conversation
There was a problem hiding this comment.
Looks good, well done @buptkang! Just left minor comments about changing the ordering of import statements, once you've done that feel free to squash and merge at your convenience.
| @@ -1,4 +1,6 @@ | |||
| #if __has_include("RCTEventEmitter.h") | |||
| #import "RCTEventEmitter.h" | |||
| #elif __has_include(<React/RCTEventEmitter.h>) | |||
There was a problem hiding this comment.
Can we swap this with the statement above?
| @@ -1,5 +1,10 @@ | |||
| #import "CodePush.h" | |||
|
|
|||
| #if __has_include("RCTConvert.h") | |||
There was a problem hiding this comment.
Same here, please swap the ordering so that <> is the default
There was a problem hiding this comment.
(and below in the other file)
There was a problem hiding this comment.
Hi guys. I'm just solving probably the issue with ordering, like you did here. Can you please explain why ordering in if ... else statement is important? Just don't get it, but seems to be causing duplicate imports. Many thanks.
There was a problem hiding this comment.
I think this was more of an aesthetic preference than anything else (put the case we expect to use first), so there wasn't necessarily a specific technical reason for it.
With the error you're facing, I'm not sure without additional investigation - but I could hypothesize that both "RCTConvert.h" and <React/RCTConvert.h> exist in 0.40 and later, and you're meant to use the latter.
There was a problem hiding this comment.
I could hypothesize that both "RCTConvert.h" and <React/RCTConvert.h> exist in 0.40
actually that is what i thought too, but find node_modules/ -name RCTBridge* finds only occurrence. Also even if there were two of them, I though that #if / #else is safe enough to not allow importing file more than once.
I saw this commit, where you're changing order and I though, you had same issue with duplicate imports, but if it's just cosmetic, then it remains mystery for me.
Anyway thank you for time and answer.
There was a problem hiding this comment.
I don't think #if #else would protect you from duplicate imports. The #if only checks for existence - if it exists, it will try to import it. That's why my hypothesis is that the ordering might matter here - you should order by preference, in case both exist.
| @@ -1,5 +1,10 @@ | |||
| #import "CodePush.h" | |||
|
|
|||
| #if __has_include("RCTConvert.h") | |||
There was a problem hiding this comment.
Should we swap the order, to do #if __has_include(<React/RCTConvert.h>) (that is, always check for the 0.40 case first)? It shouldn't really matter, that that would make all the files consistent and seems slightly better.
This PR finalizes the upgrade toward RN 0.40, which relates to previous PRs: #638, #665 and #670.