Skip to content
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

fix: building without an app.json file #627

Merged
merged 1 commit into from
Aug 9, 2024

Conversation

DoctorJohn
Copy link
Contributor

@DoctorJohn DoctorJohn commented Aug 8, 2024

Description

This PR fixes builds of projects that don't have app.json file at all.

This is done by fixing two bugs:

  1. The bailout conditions added in fix: expo managed project detection #616 turned out to be tautoligical. They try to detect managed expo projects during build time by checking the existance of android and ios folders. These folders will always exist during build time rendering the checks useless. As a result bare react native projects without a valid app.json no longer fail at build time. Note that correctly configured projects are not affected by this bug at all (that's why my testing did not reveal this bug).
  2. Builds of any android app without app.json files currently fail because of a missing check for the availability of the parsed contents of that file.

Note that this PR fully removes the bailout conditions in both build.gradle and ios_config.sh. Unfortunately we have no sufficient replacement. The check used before #616 works at build time but does not work with the use case outlined in #614. Without a valid bailout condition we lose the ability to let build.gradle and ios_config.sh fail when a bare react native project does not have a valid app.json file. Instead we now show a warning during build time when no admob app ids are found in app.json or app.json does not exist at all.

Related issues

Test Plan

  • I confirmed managed expo projects build showing the expected warning and run successfully (on android)
  • I confirmed the example project builds and runs as-is (on android)
  • I confirmed the example project builds, but yields a warning if no admob app ids are provided (on android)
  • I confirmed the repro in [🐛] [IOS] Build Error after installation of this library #630 builds showing the expected warning and runs successfully (on android)

Here is how to validate that this PR actually fixes the linked issue:

git clone https://github.com/jpdriver/expo-google-ads-repro.git
cd expo-google-ads-repro
git checkout 992e726314c7855dd493ac5d3e529c4a5328e37b
yarn add "git+https://github.com/DoctorJohn/react-native-google-ads.git#hotfix-builds-without-app-json"
sed -i 's/"androidAppId": "ca-app-pub-xxxxxxxx~xxxxxxxx"/"androidAppId": "ca-app-pub-3940256099942544~3347511713"/g' app.config.js
npx expo prebuild --clean
yarn android

Note that I was only able to test on android this time. It would be benefitial if someone could verify these changes on ios too. To do so run the example app included in this repo on ios AND run the following commands in addition to the ones above to verify this PR works with the repro in the linked issue:

sed -i 's/"iosAppId": "ca-app-pub-xxxxxxxx~xxxxxxxx"/"iosAppId": "ca-app-pub-3940256099942544~1458002511"/g' app.config.js
sed -i 's/true/true,\n\t\t\t"bundleIdentifier": "com.anonymous.myapp"/g' app.config.js
npx expo prebuild --clean
yarn ios

Copy link

docs-page bot commented Aug 8, 2024

To view this pull requests documentation preview, visit the following URL:

docs.page/invertase/react-native-google-mobile-ads~627

Documentation is deployed and generated using docs.page.

Copy link

codecov bot commented Aug 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 43.26%. Comparing base (a34c7ba) to head (458fba6).
Report is 49 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #627      +/-   ##
==========================================
- Coverage   43.72%   43.26%   -0.45%     
==========================================
  Files          30       31       +1     
  Lines         549      571      +22     
  Branches      151      152       +1     
==========================================
+ Hits          240      247       +7     
- Misses        309      324      +15     

@dylancom dylancom merged commit fe0f4f2 into invertase:main Aug 9, 2024
16 of 17 checks passed
@mikehardy
Copy link
Collaborator

🎉 This PR is included in version 14.2.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[🐛] No signature of method: java.lang.Boolean.getStringValue() under android v14.1.0+
3 participants