fix(admob): AdEventHandler returns javascript (not native) unsubscribe function

[anta-semenov]

_setAdEventHandler method return a wrong unsubscribe function. It should remove js listener, but instead it unsubscribe ad instance from native events and _onAdEventHandler is still could be used by the instance

Description

RewardedAd instance onAdEvent method uses _setAdEventHandler of it's superclass. Logically it should return function that will unsubscribe listener that we add, but instead it unsubscribe instance from native events.

Related issues

It's not possible to use different listeners for load and show ad because after first unsubscribe ad intance doesn't receive any native events anymore

const rewarded = RewardedAd.createForAdRequest(TestIds.REWARDED, {
  requestNonPersonalizedAdsOnly: true,
})

const loadAd = () => {
  if ([AdLoadStatus.InProgress, AdLoadStatus.Loaded].includes(adLoadStatus)) {
    return
  }
  const eventListener = rewarded.onAdEvent((type, error) => {
    if (type === RewardedAdEventType.LOADED) {
      adLoadStatus = AdLoadStatus.Loaded
      eventListener()
    } else if (type === AdEventType.ERROR) {
      adLoadStatus = AdLoadStatus.Failed
      eventListener()
    }
  })
  rewarded.load()
}

const showAd = (callback: () => void) => {
  const eventListener = rewarded.onAdEvent((type, error) => {
    console.log('SHow Ad error ', error, type)
    if (type === RewardedAdEventType.EARNED_REWARD) {
      adLoadStatus = AdLoadStatus.Idle
      eventListener()
      callback()
    }
  })
  rewarded.show()
}

When we call loadAd on app start and then we want to call showAd at some moment it won't receive any native events

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 packages/\*\*/e2e
  • jest tests added or updated in packages/\*\*/__tests__
• [ x ] I have updated TypeScript types that are affected by my change. (there is no changes in types)
• This is a breaking change;
  • Yes
  • [ x ] No

[vercel]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/invertase/react-native-firebase/glnr6112b
✅ Preview: https://react-native-firebase-git-fork-anta-semenov-patch-1.invertase.vercel.app

[CLAassistant]

All committers have signed the CLA.

[codecov]

Codecov Report

Merging #4920 (e2edb68) into master (fa3d260) will increase coverage by 0.43%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4920      +/-   ##
==========================================
+ Coverage   88.50%   88.93%   +0.43%     
==========================================
  Files         109      109              
  Lines        3721     3721              
  Branches      348      348              
==========================================
+ Hits         3293     3309      +16     
+ Misses        385      369      -16     
  Partials       43       43              

[mikehardy]

Thanks for posting this! I just tagged it for review - only 3 items in the queue, hope to get to this quickly and either just merge or work with you to get it merged and released. Cheers

[mikehardy]

Thanks for your patience! I do not use admob personally but the problem as you describe it, the code I see plus your change make sense, and it seems reasonable. I'll merge it and it will be in the next release which should be today or tomorrow - either 10.8.1 or 10.9 depending on what else gets merged

[mikehardy]

Releasing as 10.8.1 right now, thanks!