fix(android): reject native ad load promise on failure - #873
Conversation
…ng functionality - Added error handling for `NativeAd.createForAdRequest`, rejecting promises on load failures. - Updated `ReactNativeGoogleMobileAdsNativeModule` to manage ad loading and destruction more effectively. - Enhanced documentation to reflect new error handling behavior and added details on `NativeAdLoadError`. - Updated dependencies in `yarn.lock` for various packages.
|
To preview the documentation for this pull request, visit the following URL: docs.page/invertase/react-native-google-mobile-ads~873
|
…ation - Included a code snippet demonstrating error handling for `NativeAd.createForAdRequest`. - Clarified the types of errors that can occur during ad loading.
|
|
|
|
- Destroy the loaded NativeAd when response ID is missing before rejecting - Reject Android loads with ERROR_LOAD to match iOS - Keep LoadAdError.message from the SDK instead of canned mapped text - Document createForAdRequest failures as promise rejects, not @throws - Tighten NativeAdLoadError to code ERROR_LOAD and opaque optional userInfo - Align native-ads docs and examples with the shared ERROR_LOAD contract - Fix Jest load assertion for default requestAgent and ERROR_LOAD rejects
mikehardy
left a comment
There was a problem hiding this comment.
Looks good after the follow-up commit — approving on code review.
Cannot merge yet: the CLA is still unsigned. @hoanggbao00 please sign the Contributor License Agreement so we can merge: https://cla-assistant.io/invertase/react-native-google-mobile-ads?pullRequest=873
Pre-merge: a reviewer should also do a manual device smoke of native-ad load failure paths (invalid unit / no-fill) to confirm the Android promise rejects as expected; the Jest coverage only mocks the JS bridge.
|
Hope that wasn't too presumptious of me to just tack on a commit and push but your fix in general looked great and we can merge it with just those little tweaks and the CLA signed. Cheers! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #873 +/- ##
==========================================
+ Coverage 43.72% 44.42% +0.70%
==========================================
Files 30 36 +6
Lines 549 671 +122
Branches 151 168 +17
==========================================
+ Hits 240 298 +58
- Misses 309 373 +64 🚀 New features to boost your workflow:
|
|
iOS failing for unrelated reasons, this is good to go if we get a CLA sign from @hoanggbao00 |
Description
Scope: Android
NativeAdonly. Other formats already report load failures via events.Problem:
NativeAd.createForAdRequest()never rejected on Android when the ad request failed, so.catch()never ran and the promise could hang.Fix: Align with iOS — reject the load promise on failure.
Changes:
AdListener.onAdFailedToLoad(error-code-no-fill,error-code-invalid-request, …)ERROR_LOAD)loadSettled)NativeAdLoadError+ docs for error handlingMotivation: apps need load failures to reject promptly so they can update UI, fire analytics, retry, or fall back to another provider, instead of waiting for a hung promise / timeout.
Banner / Interstitial / Rewarded / App Open already report failures via events — this gap is specific to Android native ads.
Related issues
Related to #755
Release Summary
fix(android): reject NativeAd load promise on failure (align with iOS)
Checklist
and followed the process outlined there for submitting PRs.
AndroidiOSe2etests added or updated in__tests__e2e__jesttests added or updated in__tests__Test Plan
error-code-invalid-requesterror-code-no-fillNativeAdyarn test __tests__/nativeAd.test.ts