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: not to add didLoadWithEvents in constructor #315

Merged

Conversation

zxcpoiu
Copy link
Member

@zxcpoiu zxcpoiu commented Nov 12, 2020

If user have registered:

RNCallKeep.addEventListener('didLoadWithEvents', () => {});
RNCallKeep.addEventListener('didDisplayIncomingCall', () => {});

And we currently add didLoadWithEvents in constructor as well then re-emit every events, since the handler are all executed asynchronously, user may receive same event in different places.

It would be better we leave the control to user on their own.
For example, use algorithm like in #205 or just foreach events and re-emit it.

@zxcpoiu zxcpoiu requested a review from manuquentin November 12, 2020 11:01
@zxcpoiu
Copy link
Member Author

zxcpoiu commented Nov 12, 2020

Also, register twice didLoadWithEvents may cause leaks ( this.listeners only store and remove the last handler )

@zxcpoiu
Copy link
Member Author

zxcpoiu commented Nov 12, 2020

What's the purpose to trigger listener in the constructor btw?

@sboily
Copy link
Member

sboily commented Nov 27, 2020

@manuquentin You approved it, but it's not merged. Is there is an issue to merge it?

@manuquentin manuquentin merged commit 60b2620 into react-native-webrtc:master Nov 30, 2020
@nhjuhoang
Copy link

I set up in native and it worked

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.

4 participants