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(hooks)!: automatically set isLoaded to false #199

Conversation

jeremyhalin
Copy link
Contributor

@jeremyhalin jeremyhalin commented Jul 30, 2022

Description

I find it weird that isLoaded in hooks doesn't change value when ad is shown. In fact, the value isLoaded = true after ad has been shown is wrong since a new ad has not been loaded and calling show will fail.

Related issues

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;
    • No

Test Plan

I wanted to add some tests on useRewardedAd hook but I had this error

    TypeError: RNAppModule.eventsNotifyReady is not a function

      34 |   ) {
      35 |     if (!this.ready) {
    > 36 |       RNAppModule.eventsNotifyReady(true);
         |                   ^
      37 |       this.ready = true;
      38 |     }
      39 |     RNAppModule.eventsAddListener(eventType);

      at GANativeEventEmitter.eventsNotifyReady [as addListener] (src/internal/GoogleMobileAdsNativeEventEmitter.ts:36:19)
      at addListener (src/internal/registry/nativeModule.ts:146:39)
      at subscribeToNativeModuleEvent (src/internal/registry/nativeModule.ts:124:7)
      at initialiseNativeModule (src/internal/registry/nativeModule.ts:203:10)
      at MobileAdsModule.get (src/internal/Module.ts:52:26)
      at MobileAdsModule.native [as initialize] (src/MobileAds.ts:57:17)
      at initialize (__tests__/rewarded.test.tsx:7:19)
      at tryCatch (node_modules/regenerator-runtime/runtime.js:63:40)
      at Generator.invoke [as _invoke] (node_modules/regenerator-runtime/runtime.js:294:22)
      at Generator.next (node_modules/regenerator-runtime/runtime.js:119:21)

rewarded.test.tsx

import { renderHook, act } from '@testing-library/react-hooks/native';
import MobileAds, { useRewardedAd } from '../src';

describe('Admob Rewarded', () => {
  describe('useRewardedAd() hook', function () {
    it('has false as isLoaded initial value', async function () {
      MobileAds().initialize();
      const { result } = renderHook(() => useRewardedAd('123'));

      expect(result.current.isLoaded).toBe(false);

      act(() => {
        result.current.load();
      });

      expect(result.current.isLoaded).toBe(true);
    });
  });
});

And I had plan to test if calling show will set isLoaded to false.


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

🔥

When ad is opened, automatically set `isLoaded` to false because `show` cannot be called.
@CLAassistant
Copy link

CLAassistant commented Jul 30, 2022

CLA assistant check
All committers have signed the CLA.

@Jongkeun
Copy link

Jongkeun commented Aug 10, 2022

I have the same issue.
I think, after showing ad and isLoaded value should be false because we've already used the loaded ad.
But it's still true.
Good fix! 👍

@dylancom
Copy link
Collaborator

dylancom commented Sep 2, 2022

I think we should set isLoaded to false upon closing the ad.
(Just like we already do in the MobileAd class, where you can access adInstance.loaded when not using hooks)
See: https://github.com/invertase/react-native-google-mobile-ads/blob/main/src/ads/MobileAd.ts#L98
@wjaykim what do you think?

@wjaykim
Copy link
Collaborator

wjaykim commented Sep 2, 2022

I agree. It was weird behavior. Let's make it change, but it's a breaking change, so we will have to increase major version.

@dylancom dylancom requested review from dylancom and wjaykim September 2, 2022 14:16
src/hooks/useFullScreenAd.ts Outdated Show resolved Hide resolved
docs/displaying-ads-hook.mdx Outdated Show resolved Hide resolved
@dylancom dylancom changed the title feat(hooks): automatically set isLoaded to false feat(hooks)!: automatically set isLoaded to false Sep 2, 2022
@jeremyhalin jeremyhalin requested review from dylancom and removed request for wjaykim September 6, 2022 10:38
@codecov
Copy link

codecov bot commented Sep 6, 2022

Codecov Report

Merging #199 (91de4a7) into main (d3e41e9) will not change coverage.
The diff coverage is 0.00%.

❗ Current head 91de4a7 differs from pull request most recent head 2da718a. Consider uploading reports for the commit 2da718a to get more accurate results

@@           Coverage Diff           @@
##             main     #199   +/-   ##
=======================================
  Coverage   22.59%   22.59%           
=======================================
  Files          34       34           
  Lines         806      806           
  Branches      199      199           
=======================================
  Hits          182      182           
  Misses        624      624           

@dylancom
Copy link
Collaborator

dylancom commented Sep 6, 2022

Thanks for your contribution!

@dylancom dylancom merged commit 9d0ecac into invertase:main Sep 6, 2022
github-actions bot pushed a commit that referenced this pull request Sep 6, 2022
## [8.0.0](v7.0.1...v8.0.0) (2022-09-06)

### ⚠ BREAKING CHANGES

* **hooks:** `isLoaded` becomes false after a fullscreen ad was shown.

### Features

* **hooks:** automatically set isLoaded to false ([#199](#199)) ([9d0ecac](9d0ecac))
@mikehardy
Copy link
Collaborator

🎉 This PR is included in version 8.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
## [8.0.0](invertase/react-native-google-mobile-ads@v7.0.1...v8.0.0) (2022-09-06)

### ⚠ BREAKING CHANGES

* **hooks:** `isLoaded` becomes false after a fullscreen ad was shown.

### Features

* **hooks:** automatically set isLoaded to false ([#199](invertase/react-native-google-mobile-ads#199)) ([9d0ecac](invertase/react-native-google-mobile-ads@9d0ecac))
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.

6 participants