Review Notes/Suggestions

[Salakar]

I know this repo is initially just a lift and shift of original code from RNFirebase to get it moved out and working separately but I had some notes/ideas/suggestions for future improvements/cleanup:

• Consider renaming to @invertase/react-native-google-ads to match native SDK names - I don't think this is actually AdMob anymore?
• Consider moving admob.json to the pre-existing app.json config file that RN already has - our existing gradle/pod scripts would just point to this file instead and look for say a google-ads object. (Could be one to consider for RNFB too)
  • NOTE: Looks like the iOS config scripts for this is missing anyway so this probably isn't working on iOS (?):
    • https://github.com/invertase/react-native-firebase/blob/master/packages/app/react-native.config.js#L7-L14
    • https://github.com/invertase/react-native-firebase/blob/master/packages/app/ios_config.sh
    • https://github.com/invertase/react-native-firebase/blob/master/packages/app/firebase_json.rb (this one probably not necessary though)
• Remove all trace of RNFirebase and unused internals/common files.
• Consider rewriting to TS - existing types in definition file could be used and rewrite could just implement those APIs.
• Ignore CI on changes to docs/** and docs.json.
• Consider additionally using normal detox testing to test UI elements such as interacting with an Ad or Interstitials (alongside current testing).

Not to say we need to block this for any of the above, just FYI / feedback. Thanks for getting this moved over also 🎉

[mikehardy]

• agreed on name change, it is not admob anymore no
• I like the idea of using app.json a lot! I haven't done the config hook-up yet (just raw lift+shift so far as you noted - not working) so this is easy to switch right now
• I intended to remove any reference to RNFB or Firebase in general, I'm sweeping all files via grep as I go
• I wanted to do TS as well and still want to but I'm planning 3 phases, first is raw lift-and-shift with just eradication of FB/Firebase + making truly common stuff generic and release, second is update to current SDK (this is the point where community is unblocked), third is more re-factoring, like switch to TS/Kotlin/Swift etc
• Normal detox testing is definitely the future, I'm not going to use the chrome debugger in this repo as I bring testing up, since that apparently has no future in the ecosystem

[mikehardy]

• I just pushed a commit that makes the name react-native-google-ads / RNGoogleAds etc everywhere
• commit includes switch to app.json for config

Pending:

• Detox E2E in CI
• CI ignoring docs etc
• docs (with typedoc) in docs.page config inside docs directory (pending chore: initialise as docs.page project #1 )

[dylancom]

@Salakar @mikehardy I would consider renaming the package to: react-native-google-mobile-ads or react-native-mobile-ads as the SDK is called the "Mobile Ads SDK". To initialize the SDK on the native side you'd call: MobileAds.initialize()

See:
https://developers.google.com/admob/android/sdk
https://developers.google.com/admob/android/quick-start#initialize_the_mobile_ads_sdk

[mikehardy]

@Salakar this is totally up to you, I'm pretty agnostic about naming other than being sort of lazy about it (thus having no motivation to change it myself, sorry to all involved). @dylancom you appear to feel pretty strongly "mobile" should be in the package name, you've mentioned it more than once. I do hear you

Would be nice to have a slightly shorter name if it were renamed, not sure what can be done about that while still having "react-native". If it were to change perhaps pulling it from @invertase namespace on npmjs would be nice, e.g. https://www.npmjs.com/package/react-native-google-mobile-ads (which is not taken at the moment).

[dylancom]

Because it was chosen to step away from admob and use the actual sdk name, I feel we should then properly use the sdk name.

…

Op 31 dec. 2021 om 16:44 heeft Mike Hardy ***@***.***> het volgende geschreven:  @Salakar this is totally up to you, I'm pretty agnostic about naming other than being sort of lazy about it (thus having no motivation to change it myself, sorry to all involved). @dylancom you appear to feel pretty strongly "mobile" should be in the package name, you've mentioned it more than once. I do hear you Would be nice to have a slightly shorter name if it were renamed, not sure what can be done about that while still having "react-native". If it were to change perhaps pulling it from @invertase namespace on npmjs would be nice, e.g. https://www.npmjs.com/package/react-native-google-mobile-ads (which is not taken at the moment). — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.

[dylancom]

@mikehardy I think we can close this?

[mikehardy]

Yeah pretty much rolled through and either did exactly this or similar enough