feat(ios, android): Add support for app.config.js

megacherry commented 6 months ago

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

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;
- [ ] Yes
- [x] No

Test Plan

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

:fire:

docs-page[bot] commented 6 months ago

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.

CLAassistant commented 6 months ago

All committers have signed the CLA.

codecov[bot] commented 5 months ago

Codecov Report

Merging #517 (942e222) into main (ef85d87) will decrease coverage by 0.16%. Report is 4 commits behind head on main. The diff coverage is n/a.

Additional details and impacted files

```diff @@ 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 ```

megacherry commented 5 months ago

@mikehardy: awesome, thank you. because of the failing test in the ci. It's a issue with boost and a wrong checksum, official workaround: https://github.com/facebook/react-native/issues/42180

mikehardy commented 5 months ago

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

megacherry commented 5 months ago

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

you are a hero :)

mikehardy commented 5 months ago

:tada: This PR is included in version 12.9.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

evelant commented 5 months ago

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 @expo/config to parse so app.config.ts would work as well.

evelant commented 5 months ago

Hmm actually I think this PR as-is will crash on ios build with an app.config.js because of

  if [[ ${_RN_ROOT_EXISTS} ]]; then
    if ! python3 --version >/dev/null 2>&1; then echo "python3 not found, app.json file processing error." && exit 1; fi
    _JSON_OUTPUT_BASE64=$(python3 -c 'import json,sys,base64;print(base64.b64encode(bytes(json.dumps(json.loads(open('"'${_SEARCH_RESULT}'"', '"'rb'"').read())['${_JSON_ROOT}']), '"'utf-8'"')).decode())' || echo "e30=")
  fi

which will try to load the .js file as json and fail.

PR incoming to fix this and add support for app.config.ts which is commonly used with expo

evelant commented 5 months ago

https://github.com/invertase/react-native-google-mobile-ads/pull/521

VictorioMolina commented 5 months ago

@evelant @mikehardy @megacherry On Expo, when you have both app.json and app.config.js, the final config is the one returned by app.config.js.

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 app.json looks like:

{
  "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 app.config.js is:

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

evelant commented 5 months ago

@VictorioMolina https://github.com/invertase/react-native-google-mobile-ads/pull/521 should fix this

Until that's merged I think you can roll back to the previous version

evelant commented 5 months ago

The ^ will cause it to resolve to the latest 12.x version. Set your version to 12.6.0 without the ^ and it should work.

VictorioMolina commented 5 months ago

@evelant uppss, sorry about that, didn't saw the caret lol (just ctrl+z). Thank you for bringing it to my attention.

invertase / react-native-google-mobile-ads