-
Notifications
You must be signed in to change notification settings - Fork 985
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
WIP - [FIX #4327] Migrate to react-native-firebase #4716
WIP - [FIX #4327] Migrate to react-native-firebase #4716
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found just a merge issue, but compared with my own rebase and all else looks the same.
android/app/build.gradle
Outdated
@@ -211,7 +212,7 @@ dependencies { | |||
implementation "com.android.support:appcompat-v7:26.1.0" | |||
// Force using exact RN version instead of relying on gradle dependency resolution | |||
// https://docs.gradle.org/current/userguide/introduction_dependency_management.html#sec:dependency_resolution | |||
compile ("com.facebook.react:react-native:0.55.4") { force = true } // From node_modules | |||
implementation ("com.facebook.react:react-native:0.53.3") { force = true } // From node_modules |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a merge issue, this should stay at 0.55.4 right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed, thanks
android/app/build.gradle
Outdated
@@ -212,7 +212,7 @@ dependencies { | |||
implementation "com.android.support:appcompat-v7:26.1.0" | |||
// Force using exact RN version instead of relying on gradle dependency resolution | |||
// https://docs.gradle.org/current/userguide/introduction_dependency_management.html#sec:dependency_resolution | |||
implementation ("com.facebook.react:react-native:0.53.3") { force = true } // From node_modules | |||
compile ("com.facebook.react:react-native:0.55.4") { force = true } // From node_modules |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we want to use implementation
though? From original PR: #4446 (comment)
branch PR-4716: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, now it's just a question of testing (which I imagine will be good since I tested recently with a similar rebase) and also having your opinion on the actual changes of this PR, to see if we're not introducing something unnecessary to the actual issue.
branch PR-4716: |
@@ -6,8 +6,8 @@ buildscript { | |||
jcenter() | |||
} | |||
dependencies { | |||
classpath 'com.android.tools.build:gradle:3.0.1' | |||
classpath 'com.google.gms:google-services:3.0.0' | |||
classpath 'com.android.tools.build:gradle:3.1.2' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure that this change is necessary, but not a problem
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is necessary
@@ -3,4 +3,4 @@ distributionBase=GRADLE_USER_HOME | |||
distributionPath=wrapper/dists | |||
zipStoreBase=GRADLE_USER_HOME | |||
zipStorePath=wrapper/dists | |||
distributionUrl=https\://services.gradle.org/distributions/gradle-4.1-all.zip | |||
distributionUrl=https\://services.gradle.org/distributions/gradle-4.4-all.zip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't be required as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://rnfirebase.io/docs/v4.2.x/installation/android it seems this change is necessary
@pombeirp looks fine for me |
Thanks @rasom! |
android/app/build.gradle
Outdated
@@ -211,7 +212,7 @@ dependencies { | |||
implementation "com.android.support:appcompat-v7:26.1.0" | |||
// Force using exact RN version instead of relying on gradle dependency resolution | |||
// https://docs.gradle.org/current/userguide/introduction_dependency_management.html#sec:dependency_resolution | |||
compile ("com.facebook.react:react-native:0.55.4") { force = true } // From node_modules |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previous comment applies to the use of compile
I believe. @dmitryn Can you comment?
@@ -9,6 +9,8 @@ | |||
<uses-permission android:name="android.permission.ACCESS_NETWORK_STATE" /> | |||
<uses-permission android:name="android.permission.ACCESS_WIFI_STATE" /> | |||
<uses-permission android:name="android.permission.READ_PROFILE" /> | |||
<uses-permission android:name="android.permission.RECEIVE_BOOT_COMPLETED" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want those?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -3,4 +3,4 @@ distributionBase=GRADLE_USER_HOME | |||
distributionPath=wrapper/dists | |||
zipStoreBase=GRADLE_USER_HOME | |||
zipStorePath=wrapper/dists | |||
distributionUrl=https\://services.gradle.org/distributions/gradle-4.1-all.zip | |||
distributionUrl=https\://services.gradle.org/distributions/gradle-4.4-all.zip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove it?
|
||
COCOAPODS: 1.3.1 | ||
COCOAPODS: 1.5.3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes it is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Firebase deps require higher version of cocoapods
- nanopb/decode (0.3.8) | ||
- nanopb/encode (0.3.8) | ||
- Protobuf (3.5.0) | ||
- React (0.55.4): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why changes to versions here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jeluard this file is autogenerated, hard to answer tbh. I suppose it's related to different cocoapods version
be03c23
to
1f73c25
Compare
branch PR-4716: |
branch PR-4716: |
76% of end-end tests have passed
Failed tests (7)Click to expand
Passed tests (22)Click to expand
|
branch PR-4716: |
100% of end-end tests have passed
Passed tests (29)Click to expand
|
branch PR-4716: |
1f73c25
to
40e55c8
Compare
branch PR-4716: |
93% of end-end tests have passed
Failed tests (2)Click to expand
Passed tests (27)Click to expand
|
This PR implies an upgrade to RN 0.55 which contains a regression: facebook/react-native#19339 #4829 We need to wait for a fix before merging this. |
branch PR-4716: |
@pombeirp @annadanchenko is this task still relevant o we can just close PR? |
It looks like we're already on RN 0.55 and |
this is just rebased #4446
i will close this PR after testing (just don't want to touch original codebase until we sure that i rebased everything correctly)
status: ready