-
-
Notifications
You must be signed in to change notification settings - Fork 145
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(android): forward port onAdLoaded signature for Kotlin 1.9 #560
fix(android): forward port onAdLoaded signature for Kotlin 1.9 #560
Conversation
5e9dc3d
to
5261681
Compare
If the force push should be a problem for you I have no problem with recreating the pr with another non force-pushed branch. :) |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #560 +/- ##
=======================================
Coverage 43.66% 43.66%
=======================================
Files 29 29
Lines 536 536
Branches 147 147
=======================================
Hits 234 234
Misses 302 302 |
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.
Hey there - thanks for making the PR!
I'm not bothered by a force push, I like a clean commit history as a reviewer so I personally prefer it
This looks like a good change as I commented on the original issue
I'm a little pre-occupied that it may be a breaking change that requires kotlin 1.9 but I also think just for caution we can skip the analysis and just release it as such with a release note warning like:
BREAKING CHANGE: onAdLoaded for Android changed to a Kotlin 1.9 compatible signature, this may not be compatible with kotlin 1.8 in react-native 0.73, so it may
require react-native 0.74 and kotlin 1.9 to work for you
seems prescriptive enough, and people can wait to upgrade if they like
pending compile check in CI - which may indicate it works fine on kotlin 1.8...
It passed android build with react-native 0.72 in use and kotlin 1.7.x so I'm going to call this backwards compatible! No worries about compatibility I can see then - hopefully not famous last words |
🎉 This PR is included in version 13.2.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Description
I was unable to build this and nobody else wanted to make a PR in the issue. I hope this resolves that issue. But the change has worked for me.
This PR fixes the android build issue: ReactNativeGoogleMobileAdsFullScreenAdModule.kt 'onAdLoaded' overrides nothing
Related issues
#511
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