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

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

Merged
merged 13 commits into from
Jan 8, 2024

Conversation

megacherry
Copy link
Contributor

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.
    • Yes
  • My change supports the following platforms;
    • Android
    • 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
    • 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

🔥

Copy link

docs-page bot commented Jan 2, 2024

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
Copy link

CLAassistant commented Jan 2, 2024

CLA assistant check
All committers have signed the CLA.

@megacherry megacherry changed the title Add support for app.config.js feat(ios, android): Add support for app.config.js Jan 2, 2024
Copy link
Collaborator

@mikehardy mikehardy left a 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

android/app-json.gradle Outdated Show resolved Hide resolved
android/build.gradle Outdated Show resolved Hide resolved
docs/index.mdx Outdated Show resolved Hide resolved
docs/index.mdx Outdated Show resolved Hide resolved
docs/index.mdx Outdated Show resolved Hide resolved
docs/index.mdx Outdated Show resolved Hide resolved
docs/index.mdx Outdated Show resolved Hide resolved
Copy link

codecov bot commented Jan 8, 2024

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
@@            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
Copy link
Contributor Author

@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
@mikehardy
Copy link
Collaborator

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
@mikehardy mikehardy merged commit be39d5a into invertase:main Jan 8, 2024
11 of 12 checks passed
@megacherry
Copy link
Contributor Author

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 :)

github-actions bot pushed a commit that referenced this pull request Jan 9, 2024
## [12.9.0](v12.8.0...v12.9.0) (2024-01-09)

### Features

* **ios, android:** Add support for app.config.js ([#517](#517)) ([be39d5a](be39d5a))

### Reverts

* Revert "chore(docs): update gathering consent info" ([a82412b](a82412b))
@mikehardy
Copy link
Collaborator

🎉 This PR is included in version 12.9.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@evelant
Copy link

evelant commented Jan 9, 2024

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
Copy link

evelant commented Jan 9, 2024

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
Copy link

evelant commented Jan 9, 2024

#521

@VictorioMolina
Copy link

VictorioMolina commented Jan 11, 2024

@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
Copy link

evelant commented Jan 11, 2024

@VictorioMolina #521 should fix this

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

@evelant
Copy link

evelant commented Jan 11, 2024

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
Copy link

VictorioMolina commented Jan 11, 2024

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

kyunkakata pushed a commit to kyunkakata/react-native-google-mobile-ads that referenced this pull request Jan 18, 2024
* 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>
kyunkakata pushed a commit to kyunkakata/react-native-google-mobile-ads that referenced this pull request Jan 18, 2024
## [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))
Star-dev325 added a commit to Star-dev325/react-native-google-mobile-ads that referenced this pull request Jun 7, 2024
## [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))
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.

None yet

5 participants