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

BrodaNoel commented 1 month ago

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

CLAassistant commented 1 month ago

All committers have signed the CLA.

DoctorJohn commented 1 week ago

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.

BrodaNoel commented 1 week ago

@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

invertase / react-native-google-mobile-ads