Socket.prototype.start & Event Emitters

[peers]

Socket.prototype.start & Event Emitters

[michelle]

Right, the initial reason we went with both was because we thought using XHR would make it faster to establish an initial connection. This is still true in a few cases, but only when the developer calls .connect(...) immediately after creating the Peer object. Are you suggesting that we should either do one or the other? Or that the flow in general could be better?

Would you suggest we have an EventEmitter in each instance to use, rather than have the instances inherit?

These are great suggestions!

[hnry]

Mainly, it's whether you will take code changes, because this can come down to personal taste.

Remove or use less of EventEmitter when it doesn't make sense to use it. That is I feel there is an over reliance on just using events.

For example would you take this pull request?
https://github.com/lovebear/peerjs/commit/1b317440936b3c9d422c9531f29af8b9a8475a8c
(there are plenty of ways to do this, the general idea is you remove eventemitter from socket)

I feel event emitter is better suited for when you aren't coupling your code together (or pub/sub pattern, but Socket is 1:1 to Peer). But since you are using it that way I don't feel like it makes sense to use it.
EventEmitter's aren't as free compared to just calling the function you want either. So there is some performance consideration.

And this isn't entirely what I mean (not specifically about EventEmitter), but just refactoring code to make functions more single use, than multi purpose.

Though I know, it can be a matter of taste.

[hnry]

Regarding the websocket / xhr , actually I think that may be a misunderstanding on my part.

Because I haven't looked at the Peer server.

Though I was playing with PeerJS and noticed it took a long time to broker peers.

So I thought the Socket code was just doing a complete switch between the two, rather than what I would've liked to see, use xhr and then hand off to websocket once its established.

[ericz]

Yeah it definitely is using xhr before WS is established.
On Mar 2, 2013 10:31 PM, "lovebear" [email protected] wrote:

Regarding the websocket / xhr , actually I think that may be a
misunderstanding on my part. I haven't looked at the Peer server. I was
playing with PeerJS and noticed it took a long time to broker peers. So I
thought the Socket code was trying to 'decide' which connected first,
websocket or xhr, rather than what I would've liked to see, use xhr and
then hand off to websocket once its established.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/22#issuecomment-14342862
.

[ericz]

I think it does make sense to
Stop using eventemitter on socket. Peer and socket are too tightly coupled
anyways, this will be more performant
On Mar 2, 2013 10:06 PM, "lovebear" [email protected] wrote:

Mainly, it's whether you will take code changes, because this can come
down to personal taste.

Remove or use less of EventEmitter when it doesn't make sense to use it.
That is I feel there is an over reliance on just using events.

For example would you take this pull request?
lovebear@1b31744https://github.com/lovebear/peerjs/commit/1b317440936b3c9d422c9531f29af8b9a8475a8c

I feel event emitter is better suited for when you aren't coupling your
code together. But since you are using it that way I don't feel like it
makes sense to do it that.
EventEmitter's aren't as free compared to just calling the function you
want either. So there is some performance consideration.

Though I know, it can be a matter of taste.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/22#issuecomment-14342670
.

[hnry]

it's not so much eventemitter itself than it is, code gets repeated, overlap, Peer<->Socket<->DataConnection all know about each other and the library API is coming from all sorts of places it feels like.

Anyway I just wanted to know if you guys agreed (and if you guys would take code changes, as I know some people will feel its personal preference).

[michelle]

@LoveBear As for your particular revision, having a reference to Peer in Socket sort of ruins the abstraction that Socket provides, so something to consider in terms of taking out EventEmitter is to have socket.onmessage(...) and socket.onerror(...), which are set after the Socket is created in Peer, which would be much lighter than having Socket inherit from EventEmitter.

I am still undecided about how I feel about this though, since node.js does use the EventEmitter pattern internally.

[hnry]

Only when it makes sense to, it provides a clean way of exposing API.

With this way it provides cleaner code to me. Again this isn't the only
way, it's more to show you don't really need event emitter as much as you
feel you do.

Also it's cleaner because it reduces the need to set Socket handlers in
Peer (which is not the point of Peer, that's the abstraction). Or having to have a function that sets other functions during runtime.

Also it would be faster, event emitter isn't free. (Though it isn't in a big deal since it's a few extra microseconds at worst.)

It does come down to preference, so you can close this issue.