search

invertase / react-native-firebase-starter

DEPRECATED: For RNFB v5 only.
https://rnfirebase.io/docs
Other
1.17k stars 334 forks source link

Update/react native60 #135

Closed makedirectory closed 4 years ago

makedirectory commented 4 years ago

Happy to leave comments if necessary on what was changed/updated

CLAassistant commented 4 years ago

CLA assistant check
All committers have signed the CLA.

makedirectory commented 4 years ago

iOS testers may need to manually modify the project configurations here if receiving an error related to RCTLog not found:

Screen Shot 2019-09-08 at 4 33 57 PM

RNFirebaseStarterTests for both debug and release should be as follows

Screen Shot 2019-09-08 at 4 36 07 PM
mikehardy commented 4 years ago

This is fantastic, and sorely needed, thank you! I won't have time to review it for a couple days while I handle a react-native-device-info major release, but if I test it locally and it works I'll be +1 pretty easy

One question about your follow on hint that iOS testers might need to adjust things - that wasn't the case with the rn59 version, is there something inherent in rn60 that makes manual adjustment necessary? I might be missing a subtlety but I don't know why that would be needed except maybe for setting development teams...

makedirectory commented 4 years ago

One question about your follow on hint that iOS testers might need to adjust things - that wasn't the case with the rn59 version, is there something inherent in rn60 that makes manual adjustment necessary? I might be missing a subtlety but I don't know why that would be needed except maybe for setting development teams...

I believe this has something to do with the use of Pods for linking React dependencies. Every time I do a pod install the config for the Test project is reset too 'None'. So that's why I've included the comment above. This may be a new issue with RN due to the switch but I'm not familiar enough to file an issue, if the consensus determines this is an issue I'm happy to do so.

A few other things to note:

(1) I could not get Android version to build without adjusting org.gradle.jvmargs within gradle.properties. I set it to 4096m but this may be able to be set lower.

(2) For Android you will need to include the Google Ads ID or the build will crash without loading the bundle. Little hard to trace this, had to use adb logcat to view the device logs.

(3) On Android, the RNFirebase repo is no longer linked in /app/build.gradle or in the MainApplication.java but I didn't remove them, just comment out.

(4) I didn't add a react-native-config.js file to the root of the application because there is no rnpm configuration in place. If you want this added, please let me know.

(5) On iOS, I didn't remove the specific versions for the Firebase Pods but I think that can be done without any issues followed by a pod update - happy to add this to the PR.

Apologies for making one large commit, I should have broken it down into multiple. If I can think of anything else I will be sure to leave an additional comment.

mikehardy commented 4 years ago

One question about your follow on hint that iOS testers might need to adjust things - that wasn't the case with the rn59 version, is there something inherent in rn60 that makes manual adjustment necessary? I might be missing a subtlety but I don't know why that would be needed except maybe for setting development teams...

I believe this has something to do with the use of Pods for linking React dependencies. Every time I do a pod install the config for the Test project is reset too 'None'. So that's why I've included the comment above. This may be a new issue with RN due to the switch but I'm not familiar enough to file an issue, if the consensus determines this is an issue I'm happy to do so.

Interesting - as long as it is documented very clearly in the README I don't mind

A few other things to note:

(1) I could not get Android version to build without adjusting org.gradle.jvmargs within gradle.properties. I set it to 4096m but this may be able to be set lower.

I commented on that I think 768 is alright

(2) For Android you will need to include the Google Ads ID or the build will crash without loading the bundle. Little hard to trace this, had to use adb logcat to view the device logs.

If you look at https://github.com/mikehardy/rnfbdemo/blob/master/make-demo.sh I just put the official sample admob ids in there and it works great. That one is a snag that catches people out, yes. Even harder to figure out on iOS where logging is harder to obtain

(3) On Android, the RNFirebase repo is no longer linked in /app/build.gradle or in the MainApplication.java but I didn't remove them, just comment out.

I'd just remove them, them but you still have to have it in settings.gradle so you can pull in the objects in MainApplication for the modules, plus to implement messaging or whatever

(4) I didn't add a react-native-config.js file to the root of the application because there is no rnpm configuration in place. If you want this added, please let me know.

Nah - I think that's just for overrides? I don't have one, personally - it's not needed for any of the modules I use at this point, including react-native-firebase

(5) On iOS, I didn't remove the specific versions for the Firebase Pods but I think that can be done without any issues followed by a pod update - happy to add this to the PR.

I think having specific versions is still useful but I'd make them all current as I commented

Apologies for making one large commit, I should have broken it down into multiple. If I can think of anything else I will be sure to leave an additional comment.

I'm not bothered :-), just really happy you took the time to do it, rn59->rn60 isn't the funnest conversion but people really use this starter heavily

makedirectory commented 4 years ago

I updated the readme.md file to reference the comment above regarding the test configuration for anybody that has issues. It does reset the configuration every single time a pod install is performed.

Only use the react-native-config.js file to add custom fonts, nothing else at this point and it's not needed for this project.

Do you think it would make sense to add the sample config for the firebase-ads package? IE: add <meta-data android:name="com.google.android.gms.ads.APPLICATION_ID" android:value="ca-app-pub-3940256099942544~3347511713"/> to the AndroidManifest.xml file...

Also worth noting that I added in a Run Script to the iOS build to start metro builder, will make a comment in the code for reference.

mikehardy commented 4 years ago

Do you think it would make sense to add the sample config for the firebase-ads package? IE: add <meta-data android:name="com.google.android.gms.ads.APPLICATION_ID" android:value="ca-app-pub-3940256099942544~3347511713"/> to the AndroidManifest.xml file...

Definitely - and in the Info.plist too - the goal of the starter is you follow a couple easy steps and the thing runs. A crash followed by doc-hunting will mean issues open in the repo - no fun. If you count on ad revenue, you'll figure it out soon enough to put in your ID

makedirectory commented 4 years ago

Do you think it would make sense to add the sample config for the firebase-ads package? IE: add <meta-data android:name="com.google.android.gms.ads.APPLICATION_ID" android:value="ca-app-pub-3940256099942544~3347511713"/> to the AndroidManifest.xml file...

Definitely - and in the Info.plist too - the goal of the starter is you follow a couple easy steps and the thing runs. A crash followed by doc-hunting will mean issues open in the repo - no fun. If you count on ad revenue, you'll figure it out soon enough to put in your ID

Added in https://github.com/invertase/react-native-firebase-starter/pull/135/commits/a836c29a40dc92c3823a07b7ce2af608f7cf592e and updated the readme

mikehardy commented 4 years ago

I love this - @Salakar + @Ehesp I am human and miss things, but I've scanned this well and @makedirectory has done what I think is some great work here. I'd merge it pending a second set of eyeballs

mikehardy commented 4 years ago

I'll wait for review on this for another day or so but I'll merge otherwise, as @danielfein was just tripped up by this today and I'd like to get it merged so the android/androidx transition and the link-vs-auto-link and pods transtions aren't propagated to any more people. I wouldn't wish that on anyone

mikehardy commented 4 years ago

np of course @Salakar I know you were traveling. I'll merge this then and we'll see how it goes