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

fix(android): disable coalescing of native events #646

Merged
merged 1 commit into from
Nov 13, 2024

Conversation

fredrifoUni
Copy link
Contributor

Description

Disable native event coalescing by setting canCoalesce to false. This library uses a single event definition for multiple events. If they happen almost at the same time, some events would otherwise get ignored.

Related issues

N/A

Release Summary

android: Fix issue where some native events may not be triggered

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

Test Plan


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

Copy link

docs-page bot commented Oct 16, 2024

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

docs.page/invertase/react-native-google-mobile-ads~646

Documentation is deployed and generated using docs.page.

@CLAassistant
Copy link

CLAassistant commented Oct 16, 2024

CLA assistant check
All committers have signed the CLA.

@mikehardy
Copy link
Collaborator

Wow that looks like a subtle one to bump in to and the change seems like it makes sense after looking through related items in react-native repo

A quick look for "onnativeevent" in the repo indicates this may be needed on the iOS side as well perhaps? There is a banner ad view component with event emitter that seems to hook into that machinery

@fredrifoUni
Copy link
Contributor Author

@mikehardy Hmm, it's possible that a similar patch is needed for iOS. We only ran into this issue on Android though.

Copy link

Hello 👋, this PR has been opened for more than 2 months with no activity on it.

If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing!

You have 15 days until this gets closed automatically

@github-actions github-actions bot added the stale label Nov 13, 2024
@fredrifoUni
Copy link
Contributor Author

@mikehardy Do you think it would be possible to have this merged in? I unfortunately do not have time to look into iOS as we are not having any issues with it.

@mikehardy
Copy link
Collaborator

sorry this went stale - I just scanned through the iOS stuff and I'm not sure it is necessary, RCTEvent has a base implementation that has canCoalesce return FALSE, so perhaps it doesn't coalesce by default so nothing to do
Either way, yeah, seems fine to merge. Thanks for the PR!

@mikehardy mikehardy added workflow: pending merge Waiting on CI or question responses to merge, but otherwise ready and removed stale labels Nov 13, 2024
@mikehardy mikehardy requested a review from dylancom November 13, 2024 23:07
Copy link

codecov bot commented Nov 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 43.11%. Comparing base (a34c7ba) to head (7a953c0).
Report is 69 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #646      +/-   ##
==========================================
- Coverage   43.72%   43.11%   -0.60%     
==========================================
  Files          30       31       +1     
  Lines         549      573      +24     
  Branches      151      156       +5     
==========================================
+ Hits          240      247       +7     
- Misses        309      326      +17     

@fredrifoUni
Copy link
Contributor Author

sorry this went stale - I just scanned through the iOS stuff and I'm not sure it is necessary, RCTEvent has a base implementation that has canCoalesce return FALSE, so perhaps it doesn't coalesce by default so nothing to do Either way, yeah, seems fine to merge. Thanks for the PR!

No worries. Thank you for looking into it!

@mikehardy mikehardy merged commit fce51b1 into invertase:main Nov 13, 2024
11 of 12 checks passed
@mikehardy
Copy link
Collaborator

all CI passed but iOS and this doesn't touch that so I merged and hit the publish button 🚢

cheers!

@mikehardy mikehardy removed the workflow: pending merge Waiting on CI or question responses to merge, but otherwise ready label Nov 13, 2024
github-actions bot pushed a commit that referenced this pull request Nov 13, 2024
### [14.3.1](v14.3.0...v14.3.1) (2024-11-13)

### Bug Fixes

* **android:** disable coalescing of native events ([#646](#646)) ([fce51b1](fce51b1))
@mikehardy
Copy link
Collaborator

🎉 This PR is included in version 14.3.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

3 participants