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 event listener conflicts #412

Merged
merged 6 commits into from
Mar 21, 2024

Conversation

123mpozzi
Copy link
Contributor

@123mpozzi 123mpozzi commented Mar 19, 2024

Description

The names we are using for events are too generic (eg. onSeek): this may cause conflicts with other 3rd party libraries in case both are included in the same project.

Note
The Android part did not have conflict issues. However, the bubbled events have been renamed also there to keep them synced with the iOS part, to avoid future confusion.

Changes

  • Rename the bubbled listeners which are communicating with our native SDKs using a onBmp prefix instead of the generic on prefix

    I decided to keep using a prefix starting with on(instead of eg. bmpOn) because in iOS RCTBubblingEventBlock must be prefixed with on.

  • Create a copy of events.ts named nativeEvents.ts which contains the bubbled event listeners

    The original file now contains the un-bubbled event listeners together with the un-bubbling function

Testing

Run integration tests to check if events are working correctly

Checklist

  • 🗒 CHANGELOG entry

@123mpozzi 123mpozzi self-assigned this Mar 19, 2024
@123mpozzi 123mpozzi marked this pull request as ready for review March 20, 2024 10:19
@123mpozzi 123mpozzi requested review from a team as code owners March 20, 2024 10:19
Copy link
Contributor

@matamegger matamegger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Android side looks fine!

Have you also done some manual expectation of the sample apps on iOS and android? :)
Do all of them still work?

@123mpozzi
Copy link
Contributor Author

Android side looks fine!

Have you also done some manual expectation of the sample apps on iOS and android? :) Do all of them still work?

Yes 👍 , I tested all the samples apart from casting and offline

Copy link
Contributor

@rolandkakonyi rolandkakonyi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good!
Manual testing looks good as well!

Please delete the testing related branches once merging this!

CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Roland Kákonyi <roland.kakonyi@bitmovin.com>
@123mpozzi
Copy link
Contributor Author

Changes look good! Manual testing looks good as well!

Please delete the testing related branches once merging this!

Sure, I will 👍

@123mpozzi 123mpozzi requested a review from rolandkakonyi March 21, 2024 13:29
@123mpozzi 123mpozzi merged commit b07a53c into development Mar 21, 2024
9 checks passed
@123mpozzi 123mpozzi deleted the feature/PRN-108-fix-event-listener-conflicts branch March 21, 2024 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants