Skip to content

Conversation

lpinca
Copy link
Member

@lpinca lpinca commented Dec 20, 2016

This merges receiver.onbinary and receiver.ontext into receiver.onmessage.
I think thank using 2 different handlers is pointless.

This is tagged as "major" because it changes the Receiver interface but I don't think many people are using the Receiver and Sender classes directly. Their interface is not documented and imo they should be private and not used directly. Unfortunately they are exposed as static properties of the WebSocket class.

I was also thinking that maybe it makes sense to emit to the WebSocket instance directly instead of using these wrapper functions.

For example we could make the Receiver constructor accept a WebSocket instance, the one the receiver is bound to.

class Receiver {
  constructor (websocket, extensions, maxPayload) {
    this.websocket = websocket;
    ...
  }
}

and then instead of calling the wrapper functions like this, emit the event directly on the websocket object.

this.websocket.emit('message', buf, { binary: true, masked: this.masked });

@lpinca
Copy link
Member Author

lpinca commented Dec 20, 2016

I hate that coveralls reports that coverage decreased only because there are fewer lines of code 😑.

@3rd-Eden
Copy link
Member

tsk tsk, shame on you @lpinca for writing better code ;-)

@3rd-Eden 3rd-Eden merged commit 3c68d33 into master Dec 20, 2016
@3rd-Eden 3rd-Eden deleted the merge/onbinary-ontext branch December 20, 2016 14:46
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.

2 participants