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(android, sdks)!: update to the latest gms:play-services-ads:20.5.0 #32

Merged
merged 18 commits into from
Dec 12, 2021
Merged

feat(android, sdks)!: update to the latest gms:play-services-ads:20.5.0 #32

merged 18 commits into from
Dec 12, 2021

Conversation

Mikenso
Copy link
Contributor

@Mikenso Mikenso commented Dec 11, 2021

Description

Android Update Banner, Interstitial, Rewarded add to be compatible with com.google.android.gms:play-services-ads:20.5.0

BREAKING CHANGES:
Please refer to upstream guides for suggestions on new usage. https://developers.google.com/admob/ios/migration and https://developers.google.com/admob/android/migration

  • compileSdkVersion now 31, change your app android build.gradle to 31 if you have not already. Note that JDK11 is required for stable compilation on compileSdkVersion 31, JDK8 has internal compiler errors with SDK31
  • onAdLeftApplication removed from the underlying SDK, use react-native built in AppState to determine app went to background
  • Smart banner ads removed; use adaptive banner ads. Set height/width explicitly taking into account device size

@CLAassistant
Copy link

CLAassistant commented Dec 11, 2021

CLA assistant check
All committers have signed the CLA.

@docs-page
Copy link

docs-page bot commented Dec 11, 2021

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

docs.page/invertase/react-native-google-ads~32

Documentation is deployed and generated using docs.page.

@mikehardy mikehardy changed the title Android update to the latest gms:play-services-ads:20.5.0 feat(android, sdks): update to the latest gms:play-services-ads:20.5.0 Dec 11, 2021
@codecov
Copy link

codecov bot commented Dec 11, 2021

Codecov Report

Merging #32 (05511a8) into main (382f146) will not change coverage.
The diff coverage is n/a.

❗ Current head 05511a8 differs from pull request most recent head 4e51dac. Consider uploading reports for the commit 4e51dac to get more accurate results

@@           Coverage Diff           @@
##             main      #32   +/-   ##
=======================================
  Coverage   19.70%   19.70%           
=======================================
  Files          21       21           
  Lines         665      665           
  Branches      167      167           
=======================================
  Hits          131      131           
  Misses        415      415           
  Partials      119      119           

@mikehardy
Copy link
Collaborator

Oh fantastic! That was next up on the todo list and it appears you have nailed it! Is this a breaking change? I did not see any changes to the javsascript layer so this appears to be all on the native side. Is that a correct guess? I have not reviewed though - just skimmed really quickly before approving the CI run

Can you sign the CLA agreement as well? Excited to move quickly on this one, and I really appreciate that you posted this

@RodolfoGS
Copy link

onAdLeftApplication will be removed? So, we'll not be able to know when a user left the app after tapped on an ad?

@mikehardy
Copy link
Collaborator

@RodolfoGS check the underlying SDK docs for reference https://developers.google.com/admob/android/migration
what do they say on the matter?

@RodolfoGS
Copy link

Mmm yes, I see, Leave application callback removal. Ok, thanks for the clarification!

@mikehardy
Copy link
Collaborator

no problem - I have not read it much myself so I was not actually sure, but I had the link handy ;-). Cheers

@mikehardy mikehardy changed the title feat(android, sdks): update to the latest gms:play-services-ads:20.5.0 feat(android, sdks)!: update to the latest gms:play-services-ads:20.5.0 Dec 11, 2021
@mikehardy
Copy link
Collaborator

I marked this as a breaking change (with a title update on the PR including !) and that means we need to generate an excellent note for library consumers indicating what the breaking change(s) is/are, and how they need to react.

It seems like this one might simply be:

BREAKING CHANGE: onAdLeftApplication is removed from the underlying SDK, there is no replacement

@mikehardy
Copy link
Collaborator

mikehardy commented Dec 11, 2021

@Mikenso also needs a bump to compileSdkVersion to 31 (that will be in the example app https://github.com/invertase/react-native-google-ads/blob/0770c96714a84725084acf4130e3c6a40b6d079d/example/android/build.gradle#L8 - unfortunately this means we also need to encourage use of JDK11 as compileSdkVersion 31 has internal compiler errors on JDK8)

Execution failed for task ':app:checkDebugAarMetadata'.
> A failure occurred while executing com.android.build.gradle.internal.tasks.CheckAarMetadataWorkAction
   > The minCompileSdk (31) specified in a
     dependency's AAR metadata (META-INF/com/android/build/gradle/aar-metadata.properties)
     is greater than this module's compileSdkVersion (android-30).
     Dependency: androidx.work:work-runtime:2.7.0.
     AAR metadata file: /Users/runner/.gradle/caches/transforms-3/4739effbb596877ca3318d6c0a41cf71/transformed/work-runtime-2.7.0/META-INF/com/android/build/gradle/aar-metadata.properties.

BREAKING CHANGE: compileSdkVersion now 31, change your app android build.gradle to 31 if you have not already. Note that JDK11 is required for stable compilation on compileSdkVersion 31, JDK8 has internal compiler errors with SDK31

@mikehardy
Copy link
Collaborator

mikehardy commented Dec 11, 2021

@Mikenso run google java formatter locally and commit the result, it will fix the lint errors:


Error: google-java-format exited with exit code 1.
    at errorFromExitCode (/home/runner/work/react-native-google-ads/react-native-google-ads/node_modules/google-java-format/index.js:19:10)
    at ChildProcess.<anonymous> (/home/runner/work/react-native-google-ads/react-native-google-ads/node_modules/google-java-format/index.js:121:40)
    at ChildProcess.emit (events.js:400:28)
    at maybeClose (internal/child_process.js:1058:16)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:293:5)
error Command failed with exit code 1.

https://github.com/invertase/react-native-google-ads/blob/0770c96714a84725084acf4130e3c6a40b6d079d/package.json#L58

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.

  • lint errors
  • example app update to compileSdkVersion 31
  • needs a crystal clear description of all breaking changes, to help app developers deal with the changes

@dylancom
Copy link
Collaborator

I think that also here setting testDevices has to move to the overall configuration instead of per ad. And so it has to be set in: https://github.com/invertase/react-native-google-ads/blob/9569b98768cacd3f0d3e95b7b44d15d20e59681a/android/src/main/java/io/invertase/googleads/ReactNativeGoogleAdsModule.java

Besides that the key "testDevices" became: "testDeviceIdentifiers".

So this PR should match with setting test devices as follow:
admob().setRequestConfiguration({testDeviceIdentifiers: ["EMULATOR"]});

@mikehardy
Copy link
Collaborator

Agreed with Dylan's comment on making sure the test device stuff is in harmony with iOS changes as mentioned, thanks for the review Dylan

@Mikenso
Copy link
Contributor Author

Mikenso commented Dec 12, 2021

Hi @mikehardy, @dylancom, thanks for review

Where do I need to specify breaking changes?
It would be:

BREAKING CHANGES:

  • compileSdkVersion now 31, change your app android build.gradle to 31 if you have not already. Note that JDK11 is required for stable compilation on compileSdkVersion 31, JDK8 has internal compiler errors with SDK31
  • onAdLeftApplication is removed from the underlying SDK, there is no replacement
  • Smart banner ads are deprecated in favor of adaptive banner ads.

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.

Starting to look good - I still need to do a deeper look (in progress) but here's the results of a surface level scan - simple item of annotation style

@mikehardy
Copy link
Collaborator

Looks like it needs either yarn lint:prettier:fix or yarn:eslint:fix as well. The lint tasks need to pass


$ yarn lint:js && yarn lint:android && yarn lint:ios:check
$ eslint lib/ --ext .js,.jsx,.ts,.tsx --max-warnings=0

/home/runner/work/react-native-google-ads/react-native-google-ads/lib/index.d.ts
Error:   123:24  error  Delete `····`  prettier/prettier

✖ 1 problem (1 error, 0 warnings)
  1 error and 0 warnings potentially fixable with the `--fix` option.

@mikehardy mikehardy merged commit 291e504 into invertase:main Dec 12, 2021
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.

looked good - I confirmed smart banners were gone on ios side as well, and left-app event was also gone on ios, so API is now consistent across platforms with both up to date on SDKs
This looks like a solid forward port, thank you

@mikehardy mikehardy removed the workflow: waiting for feedback Waiting on response to questions label Dec 12, 2021
github-actions bot pushed a commit that referenced this pull request Dec 12, 2021
## [3.0.0](v2.0.1...v3.0.0) (2021-12-12)

### ⚠ BREAKING CHANGES

* android SDK updated to underlying SDK 20
* **android, sdks:** update to the latest v20 android admob sdk (#32)

### Features

* android SDK updated to underlying SDK 20 ([56c6058](56c6058))
* **android, sdks:** update to the latest v20 android admob sdk ([#32](#32)) ([291e504](291e504))
@mikehardy
Copy link
Collaborator

🎉 This PR is included in version 3.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Star-dev325 added a commit to Star-dev325/react-native-google-mobile-ads that referenced this pull request Jun 7, 2024
## [3.0.0](invertase/react-native-google-mobile-ads@v2.0.1...v3.0.0) (2021-12-12)

### ⚠ BREAKING CHANGES

* android SDK updated to underlying SDK 20
* **android, sdks:** update to the latest v20 android admob sdk (#32)

### Features

* android SDK updated to underlying SDK 20 ([56c6058](invertase/react-native-google-mobile-ads@56c6058))
* **android, sdks:** update to the latest v20 android admob sdk ([#32](invertase/react-native-google-mobile-ads#32)) ([291e504](invertase/react-native-google-mobile-ads@291e504))
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.

5 participants