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.

Sign up for GitHub

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

Jump to bottom

added roomName in createdPeer event #405

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

added roomName in createdPeer event #405

wants to merge 16 commits into from

Conversation

csturiale
Copy link

createdPeer is an event called for each joined room then I suppose that is better to have roomName as parameter in the callback to manage presence or autosend message in the room channel better.

@fippo
Copy link
Contributor

fippo commented Mar 7, 2016

that looks useful. Can you revert the changes to package.json and the bundle (we do that after merging) and squash your commits into a single one please?

@csturiale
Copy link
Author

done

@xdumaine
Copy link
Contributor

xdumaine commented Mar 7, 2016

@csturiale it doesn't look like you updated. Maybe you forgot to push?

added roomName in createdPeer event (+1 squashed commit)
5ac5de5 
Squashed commits:
[7ab0253] revert to package.json and bundle (+3 squashed commits)
Squashed commits:
[9532b6f] 2.1.2
[cb97b07] make latest-v2 and bundle
[ceed4e6] 2.1.1
@csturiale
Copy link
Author

sorry I think it is ok now

@fippo
Copy link
Contributor

fippo commented Mar 19, 2016

looked at it again. Adding it in just one of the three events would be very inconsistent.

And I don't understand the use-case. One of the underlying assumptions is that you're not in more than a single room.

@loureirorg
Copy link

Has this been merged to master? To me, the roomName is always "simplewebrtc".

Zythyr
Zythyr reviewed Apr 18, 2018
View reviewed changes
Copy link
Contributor

@Zythyr Zythyr left a comment

Choose a reason for hiding this comment

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

The goal was so that 'createdPeer' event emits the roomName in addition to the peer info. However the emit on line 350 doesn't include the roomName.

simplewebrtc.js
@@ -346,7 +348,7 @@ SimpleWebRTC.prototype.joinRoom = function (name, cb) {
}
});
self.emit('createdPeer', peer);
Copy link
Contributor

@Zythyr Zythyr Apr 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

self.emit('createdPeer', peer, roomName);

@Zythyr
Copy link
Contributor

Zythyr commented Apr 18, 2018

In this issue (https://github.com/andyet/SimpleWebRTC/issues/692) I reported that createdPeer get emitted when a peer is already connect to the room and tries to connect again. Thus this event gets duplicate emits. Should we also update the code to prevent this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Zythyr Zythyr Zythyr left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@csturiale @fippo @xdumaine @loureirorg @Zythyr @daniloarcidiacono