react-native-google-mobile-ads icon indicating copy to clipboard operation
react-native-google-mobile-ads copied to clipboard

feat!: [BREAKING CHANGE]: Use `extra` instead of root

Open BrodaNoel opened this issue 1 year ago • 3 comments

Description

This PR tries to fix https://github.com/invertase/react-native-google-mobile-ads/issues/581. All the description of this problem is defined there, but, basically, since Expo SDK 51, it won't work if we try to get the react-native-google-mobile-ads key from the root of app.json. We need to get it from root.extra (thus, we need to change this documentation as well)

Please don't kill me... I have no idea what I am doing in that file (it was actually 100% gpt-4o).

The behavior implemented is this: First, tries to find the react-native-google-mobile-ads key inside root > expo > extra, then, if it doesn't find it, it tries in root > extra (because Expo Cli may merge it soon).

Pending stuff

We'll need to also change the documentation of this library, asking the users to put react-native-google-mobile-ads inside extra, instead of directly inside the root.

Related issues

Fixes #581

Release Summary

Breaking: Move the react-native-google-mobile-ads key in app.json from root to root.extra

Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
    • [x] Yes
  • My change supports the following platforms;
    • [x] Android
    • [x] iOS
  • My change includes tests;
    • [ ] e2e tests added or updated in __tests__e2e__
    • [ ] jest tests added or updated in __tests__
  • [ ] I have updated TypeScript types that are affected by my change.
  • This is a breaking change;
    • [x] Yes
    • [ ] No

Test Plan

I'm not sure if this works. I'm proposing this PR as a first initial version


Think react-native-google-mobile-ads is great? Please consider supporting the project with any of the below:

  • 👉 Star this repo on GitHub ⭐️
  • 👉 Follow Invertase on Twitter

BrodaNoel avatar May 29 '24 01:05 BrodaNoel

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar May 29 '24 01:05 CLAassistant

basically, since Expo SDK 51, it won't work

I'm not sure that's true. Even in the referenced issue some people say it doesn't work with SDK 50 either. From my understanding nothing relevant changed in expo but PR #517 broke loading of the react-native-google-mobile-ads config from app.json when there is also an app.config.js file present. PR #521 proposes a fix for this but also adds additional features which make it unlikely it will ever be merged.

I think we need to reevaluate how we want to insert this packages config into native code. Ideally we would have a config plugin for expo but that would conflict with the ios_config.sh and the app-json.gradle files which implement a similar mechanism.

DoctorJohn avatar Jun 17 '24 08:06 DoctorJohn

@DoctorJohn actually there is something who was changed on SDK 51 who broke it. Read this: https://github.com/expo/fyi/blob/main/root-expo-object.md#migrating-the-config

BrodaNoel avatar Jun 17 '24 08:06 BrodaNoel

:tada: This issue has been resolved in version 14.0.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

mikehardy avatar Jul 12 '24 08:07 mikehardy

Has the config changed at all in expo? or is app.json still setup same as written in documentation?

🎉 This issue has been resolved in version 14.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

longtimeno-c avatar Jul 12 '24 12:07 longtimeno-c

Scherm­afbeelding 2024-07-12 om 14 52 20

dylancom avatar Jul 12 '24 12:07 dylancom