-
-
Notifications
You must be signed in to change notification settings - Fork 153
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
feat(ios, android): Add support for app.config.js #517
feat(ios, android): Add support for app.config.js #517
Conversation
To view this pull requests documentation preview, visit the following URL: docs.page/invertase/react-native-google-mobile-ads~517 Documentation is deployed and generated using docs.page. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all looks technically correct, awesome
I'll pull it locally to make sure it builds and configures the example correctly but I don't imagine there will be problems
My only proposed changes are with regards to documentation, three things none of which are functional:
- just a little explanation in the gradle scripting in the form of a comment describing how it works after I reasoned through how it works so I don't have to reason through it again ;-)
- splitting the
app.json/app.config.js
comments into an "or" instead of the possibly ambiguous "/" so people don't think they have to do both - removing the blob of sdkadnetwork strings from the proposed docs for app.config.js - those change semi-frequently and if they are in two places I'm certain one will be forgotten in the future. Better to just reference one blob with a prescriptive comment
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #517 +/- ##
==========================================
- Coverage 43.99% 43.83% -0.16%
==========================================
Files 29 29
Lines 532 534 +2
Branches 147 147
==========================================
Hits 234 234
- Misses 298 300 +2 |
@mikehardy: awesome, thank you. because of the failing test in the ci. It's a issue with boost and a wrong checksum, official workaround: facebook/react-native#42180 |
- requires updated patch to handle AGP namespace requirements - requires updated patch to handle new apple sim error codes
…exception - windows needs to escape the absolute file path or config file not found - groovy procs are async, script was continuing before proc had finished leading to no output - if there is an empty config file output we can propagate that to main catch so the error message to user is clear but not duplicated
there are more problems than that but I think I've got it ironed out Some of them were definitely no fault of this PR, mainly that our example app is a bit dated - so I worked around that directly The android portion of this was definitely problematic though as it wasn't waiting for async groovy process before going for output on the node exec and it had pathing issues on Windows. I went ahead and directly fixed that + re-pushed since I learned a lot there and was happy to learn it :-) I think it's going to go green now, either way I'm doing what I think is final testing locally on windows (works) and mac (in process) to make sure ios and android are all good then can merge |
works around known upstream issue where jfrog is currently unreliable
you are a hero :) |
🎉 This PR is included in version 12.9.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Awesome stuff! Although I foresee issues being filed along the lines of "I use app.config.ts and it doesn't work". I'll see if I can make it use |
Hmm actually I think this PR as-is will crash on ios build with an app.config.js because of
which will try to load the PR incoming to fix this and add support for |
@evelant @mikehardy @megacherry On Expo, when you have both I am getting the following error when creating a development build for iOS: info: -> RNGoogleMobileAds build script started
info: 1) Locating app.json file:
info: (1 of 2) Searching in '/Users/expo/workingdir/build/packages/app' for a app.json/app.config.js file.
info: app.config.js found at /Users/expo/workingdir/build/packages/app/app.config.js
/opt/homebrew/Cellar/ruby@2.7/2.7.7/lib/ruby/2.7.0/json/common.rb:156:in `parse': 783: unexpected token at 'undefined' (JSON::ParserError)
from /opt/homebrew/Cellar/ruby@2.7/2.7.7/lib/ruby/2.7.0/json/common.rb:156:in `parse'
from -e:1:in `<main>'
info: 2) Injecting Info.plist entries:
-> 0) google_mobile_ads_json_raw string e30=
error: ios_app_id key not found in react-native-google-mobile-ads key in app.json. App will crash without it. Which is caused by this flow due to a wrong parsing of the searched file: _SEARCH_RESULT=''
_JSON_FILE_NAME='app.json'
_JS_APP_CONFIG_FILE_NAME='app.config.js'
_SEARCH_RESULT=$(find \"$_CURRENT_SEARCH_DIR\" -maxdepth 2 \\( -name ${_JSON_FILE_NAME} -o -name ${_JS_APP_CONFIG_FILE_NAME} \\) -print | /usr/bin/head -n 1)
if [[ \"$(basename ${_SEARCH_RESULT})\" = \"${_JS_APP_CONFIG_FILE_NAME}\" ]]; then\n _IS_CONFIG_JS=true\n echo \"info: ${_JS_APP_CONFIG_FILE_NAME} found at $_SEARCH_RESULT\"\n break;\n fi;
if [[ ${_IS_CONFIG_JS} == \"true\" ]]; then\n _JSON_OUTPUT_RAW=$(\"${NODE_BINARY}\" -e \"console.log(JSON.stringify(require('${_SEARCH_RESULT}')));\")\n else\n _JSON_OUTPUT_RAW=$(cat \"${_SEARCH_RESULT}\")\n fi;
_IOS_APP_ID=$(getJsonKeyValue \"$_JSON_OUTPUT_RAW\" \"ios_app_id\")
if ! [[ $_IOS_APP_ID ]]; then\n echo \"error: ios_app_id key not found in react-native-google-mobile-ads key in app.json. App will crash without it.\"\n exit 1 My current {
"expo": {
...
},
"react-native-google-mobile-ads": {
"ios_app_id": "ca-app-pub-xxx~xxx",
"android_app_id": "ca-app-pub-xxx~xxx",
"delay_app_measurement_init": true,
"user_tracking_usage_description": "This identifier will be used to deliver personalized ads to you."
}
} My require("dotenv/config");
const _ = require("lodash");
/**
* Merges the app static configuration with the dynamic one, which is also
* required for the compilation process.
*
* @param {import("expo/config").ConfigContext} options
* Static Expo configuration.
* @returns {import("expo/config").AppJSONConfig}
* Final 'App.json' configuration (merged).
*/
module.exports = ({ config: staticConfig }) => {
const dynamicConfig = {
expo: {
ios: {
googleServicesFile: process.env.GOOGLE_SERVICE_INFO_PLIST,
config: {
googleMapsApiKey: process.env.GOOGLE_SERVICES_API_KEY_IOS,
},
},
android: {
googleServicesFile: process.env.GOOGLE_SERVICES_JSON,
config: {
googleMaps: {
apiKey: process.env.GOOGLE_SERVICES_API_KEY_ANDROID,
},
},
},
web: {
config: {
firebase: {
appId: process.env.FIREBASE_APP_ID,
apiKey: process.env.FIREBASE_API_KEY,
authDomain: process.env.FIREBASE_AUTH_DOMAIN,
projectId: process.env.FIREBASE_PROJECT_ID,
storageBucket: process.env.FIREBASE_STORAGE_BUCKET,
messagingSenderId: process.env.FIREBASE_MESSAGING_SENDER_ID,
measurementId: process.env.FIREBASE_MEASUREMENT_ID,
},
},
},
extra: {
eas: {
projectId: process.env.EAS_PROJECT_ID,
},
},
},
};
dynamicConfig.expo = _.merge(staticConfig, dynamicConfig.expo);
return dynamicConfig;
}; This configuration was working the last time I built my project (2 weeks ago). Please, also note that the thrown error is always displaying app.json (not considering app.config.js) in the code: if ! [[ $_IOS_APP_ID ]]; then\n echo \"error: ios_app_id key not found in react-native-google-mobile-ads key in app.json. App will crash without it.\"\n exit 1 |
@VictorioMolina #521 should fix this Until that's merged I think you can roll back to the previous version |
The |
@evelant uppss, sorry about that, didn't saw the caret lol (just ctrl+z). Thank you for bringing it to my attention. |
* feat(config, ios): add support for app.config.js * feat(config, android): add support for app.config.js * docs(installation): enhance docs for app.config.js support * Update android/app-json.gradle * Update docs/index.mdx * test: pin detox to 19 (20 does not do jest) - requires updated patch to handle AGP namespace requirements - requires updated patch to handle new apple sim error codes * fix(android): windows-compat pathing, wait for async proc, propagate exception - windows needs to escape the absolute file path or config file not found - groovy procs are async, script was continuing before proc had finished leading to no output - if there is an empty config file output we can propagate that to main catch so the error message to user is clear but not duplicated * test(ios): alter boost download source works around known upstream issue where jfrog is currently unreliable --------- Co-authored-by: Mike Hardy <github@mikehardy.net>
## [12.9.0](invertase/react-native-google-mobile-ads@v12.8.0...v12.9.0) (2024-01-09) ### Features * **ios, android:** Add support for app.config.js ([invertase#517](invertase#517)) ([be39d5a](invertase@be39d5a)) ### Reverts * Revert "chore(docs): update gathering consent info" ([a82412b](invertase@a82412b))
## [12.9.0](invertase/react-native-google-mobile-ads@v12.8.0...v12.9.0) (2024-01-09) ### Features * **ios, android:** Add support for app.config.js ([#517](invertase/react-native-google-mobile-ads#517)) ([be39d5a](invertase/react-native-google-mobile-ads@be39d5a)) ### Reverts * Revert "chore(docs): update gathering consent info" ([a82412b](invertase/react-native-google-mobile-ads@a82412b))
Description
I have improved my original patch so that no additional file for ios is needed. I have added the original patch for android from #370 and tested it on android.
I have also adapted the documentation to the best of my knowledge.
Related issues
The issue for ios: #462
The issue for android: #370
Release Summary
Checklist
and followed the process outlined there for submitting PRs.
Android
iOS
e2e
tests added or updated in__tests__e2e__
jest
tests added or updated in__tests__
Test Plan
Think
react-native-google-mobile-ads
is great? Please consider supporting the project with any of the below:Invertase
on Twitter🔥