Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update react native 0.50.3 #2217

Merged
merged 1 commit into from
Dec 15, 2017
Merged

Update react native 0.50.3 #2217

merged 1 commit into from
Dec 15, 2017

Conversation

yenda
Copy link
Contributor

@yenda yenda commented Oct 19, 2017

fixes #1690

TODOS:

- [x] Android migration
- [x] iOS migration
- [x] move all github repos in package.json to Status repos

Test:
This changes react-native version and update most libraries in the process. But there is no changes to our code itself so if the app loads, tabs work, scrolling work, drawer works it should be good.

status: ready

@yenda yenda self-assigned this Oct 19, 2017
@yenda yenda mentioned this pull request Oct 24, 2017
9 tasks
@yenda yenda added the blocker label Oct 25, 2017
@@ -73,7 +74,7 @@ public boolean getUseDeveloperSupport() {
new ReactNativeDialogsPackage(),
new ImageResizerPackage(),
new PickerPackage(),
new WebViewBridgePackage(BuildConfig.DEBUG, callRPC),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We definitely need to pass callRPC to WebViewBridgePackage

@oskarth
Copy link
Contributor

oskarth commented Nov 9, 2017

@yenda What's the state of this PR?

@yenda yenda force-pushed the update-react-native branch 8 times, most recently from 1c2df27 to 53a998e Compare November 10, 2017 14:26
@yenda yenda force-pushed the update-react-native branch 2 times, most recently from 9642373 to d97c171 Compare November 11, 2017 08:04
@@ -43,6 +43,7 @@
81C6E6AE0AA739BE9D87C1D0 /* libPods-StatusImTests.a in Frameworks */ = {isa = PBXBuildFile; fileRef = FC1CBCFE6C906043D6CCEEE1 /* libPods-StatusImTests.a */; };
82E689BAF9FB43C8AC6FF1CA /* EvilIcons.ttf in Resources */ = {isa = PBXBuildFile; fileRef = CEB0E2659D1A4F5FA842057A /* EvilIcons.ttf */; };
832341BD1AAA6AB300B99B32 /* libRCTText.a in Frameworks */ = {isa = PBXBuildFile; fileRef = 832341B51AAA6A8300B99B32 /* libRCTText.a */; };
<<<<<<< ours
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merge conflicts shouldn't be committed

@oskarth
Copy link
Contributor

oskarth commented Nov 13, 2017

Please specify version of RN we are upgrading to; when issue was created latest stable was 0.47, now it is 0.50. Is this the one we use?

@yenda yenda changed the title [WIP] Update react native [WIP] Update react native 0.50.3 Nov 13, 2017
@oskarth
Copy link
Contributor

oskarth commented Nov 14, 2017

Relevant issue for iOS upgrade and unable to open Xcode: facebook/react-native#12261

@oskarth
Copy link
Contributor

oskarth commented Nov 14, 2017

  1. My removal of the merge conflicted pbxproject file was a mistake; please ignore this noop.

  2. I thought this RN iOS upgrade stuff would be fairly straightforward with all the work @yenda had already done, but it looks like it is a bunch of indeterminate work. This seems like it is within the scope of Improve developer experience on status-react swarms#9 swarm (4 devs committed), and I'd rather spend time kickstarting offline inboxing swarm since I'm leading that one.

  3. Why it seems like indeterminate work:

  • apply common sense merge (use both and make sure all structures are enclosed properly, remove duplicates) => Xcode won't open.
  • Apply only their changes => pod install => whole project file nuked
  • Long thread in official repo from February with no canonical solution using current upgrade path

So for all I know it could take a week. It might also take an hour if you find the right combination. One idea is to start RN upgrade from scratch on iOS side, don't use the magic git upgrade script, and then merge that upgrade with this one. We could also put up a bounty to get some Xcode ninjas to resolve this.

@yenda yenda force-pushed the update-react-native branch from 4f4d8fb to 89c793d Compare November 14, 2017 17:44
@janherich janherich self-assigned this Nov 16, 2017
@janherich
Copy link
Contributor

The last status - except for changes here, one needs to change the import in header files of the react-native-http-bridge and instabug-reactnative from #import "RCTBridgeModule.h" to #import <React/RCTBridgeModule.h> (see this thread about the issue - facebook/react-native#15775).
I patched them in my node_modules folder, but of course this needs to be fixed in upstream repositories.
After that, the build fails on the linker invocation, which can't find the GoogleToolboxForMaxLibrary despite the framework being there and referenced correctly in the project.pbxproj file, see screenshot:
screen shot 2017-11-22 at 00 37 55

@yenda
Copy link
Contributor Author

yenda commented Nov 22, 2017

@janherich can you fork the libs until they are fixed upstream because node_modules is local. We might want to do this for all libs anyway

@oskarth
Copy link
Contributor

oskarth commented Nov 22, 2017

I decided to attack this a bit. I didn't read your comment in detail beforehand, but I did the same thing as you did. Attached patch files which makes it reproducible.

Issue that is quickfix: #2505

Issue for thing afterwards: #2506

package.json Outdated
@@ -29,7 +29,7 @@
"homoglyph-finder": "1.1.1",
"https-browserify": "0.0.1",
"identicon.js": "github:status-im/identicon.js",
"instabug-reactnative": "git+https://github.com/status-im/instabug-reactnative.git",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How come you don't update the status-im one? We should use that, not our own forks.

If it is a permission issue and you do this to be unblocked: OK - please ping Carl and/or Nabil and he will sort you (us) out.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

package.json Outdated
@@ -29,7 +29,7 @@
"homoglyph-finder": "1.1.1",
"https-browserify": "0.0.1",
"identicon.js": "github:status-im/identicon.js",
"instabug-reactnative": "git+https://github.com/status-im/instabug-reactnative.git",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

package.json Outdated
@@ -55,7 +55,7 @@
"react-native-fcm": "10.0.3",
"react-native-fs": "2.8.1",
"react-native-http": "github:tradle/react-native-http#834492d",
"react-native-http-bridge": "github:yenda/react-native-http-bridge",
"react-native-http-bridge": "github:janherich/react-native-http-bridge",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hot potato :D

package.json Outdated
"https-browserify": "0.0.1",
"identicon.js": "github:status-im/identicon.js",
"instabug-reactnative": "git+https://github.com/status-im/instabug-reactnative.git",
"nfc-react-native": "github:status-im/nfc-react-native",
"instabug-reactnative": "git+https://github.com/janherich/instabug-reactnative.git",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment appeared as outdated, not sure why. 54f5bcb#r152777933

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this is due to permission issues, ping Carl and Nabil in slack on this

@janherich
Copy link
Contributor

@yenda everything works on iOS side (including re-frisk/figwheel in dev mode) I propose that you will pull the newest changes, make sure that everything is still working on the Android side, and we can then request comprehensive regression testing from QA.
Once this is completed, I can then squash commits, transfer forked repo changes to repositories owned by status-im organisation and finally rebase and merge it into develop :)

@yenda yenda force-pushed the update-react-native branch 2 times, most recently from c838456 to 25ec360 Compare December 8, 2017 21:44
@yenda
Copy link
Contributor Author

yenda commented Dec 8, 2017

Branch: status-im:update-react-native
Android: https://i.diawi.com/CSM6ZK
iOS: https://i.diawi.com/W9A8hm

@yenda yenda changed the title [WIP] Update react native 0.50.3 Update react native 0.50.3 Dec 8, 2017
@asemiankevich
Copy link
Contributor

asemiankevich commented Dec 11, 2017

  1. No commands shown after second launch (known, fixed in develop)
  2. No dot/comma is shown on keyboard when requesting/sending funds #2681. This is fixed in 0.9.10-438-g6948dd17+ branch
  3. Picture in user profile is not updated when selecting from gallery or capture [2217] #2682. This is fixed in 0.9.10-438-g6948dd17+ branch
  4. ios build can not be started
  5. Null is not an object (evaluating 'e.toLowerCase') error shown [2217] #2697. This is closed as duplicate of After upgrade: null is not an object (evaluating 'e.toLowerCase') if last message in chat is Request #2602. Moreover it is in develop and is not related to current branch
  6. Invariat Violation error shown when opening transaction history screen [2217] #2709

@yenda yenda force-pushed the update-react-native branch from 25ec360 to a62c1a7 Compare December 11, 2017 13:48
@yenda yenda force-pushed the update-react-native branch from 505d287 to ec74efe Compare December 13, 2017 08:55
@asemiankevich asemiankevich self-assigned this Dec 14, 2017
@asemiankevich
Copy link
Contributor

asemiankevich commented Dec 14, 2017

All issues above are fixed in branch below. No new issues found. I assume we can merge this.


Branch: status-im:update-react-native
Android: https://i.diawi.com/x1acKd
iOS: https://i.diawi.com/bwcauj

@yenda yenda force-pushed the update-react-native branch from f222940 to f472881 Compare December 15, 2017 15:03
@yenda yenda merged commit 56c59c7 into develop Dec 15, 2017
@rasom rasom deleted the update-react-native branch December 21, 2017 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade React Native from 0.43 to latest
5 participants