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

W3C API compatibility #18

Closed
aslakhellesoy opened this issue Feb 6, 2012 · 9 comments
Closed

W3C API compatibility #18

aslakhellesoy opened this issue Feb 6, 2012 · 9 comments

Comments

@aslakhellesoy
Copy link
Contributor

I'm writing a library on top of your excellent library that I'd like to use both on Node.js and in browsers. I have ran into a couple of issues with API incompatibility. The W3C API allows these two variants:

ws.onmessage = function(e) {
    var payload = e.data;
};

ws.addEventListener('message', function(e) {
    var payload = e.data;
});

Your implementation allows:

ws.on('message', function(payload){
});

I'm proposing 3 changes:

  • Deliver messages in an Event object with a data attribute
  • Allow ws.addEventListener(eventName, Function)
  • Allow ws.onmessage = Function

This would break the current API, but it would also make it follow the official API more closely. Would you be interested in a patch for this?

Aslak

@einaros
Copy link
Contributor

einaros commented Feb 6, 2012

I think that's a very natural direction for the api to take at this point.

@3rd-Eden added an emulation layer for the .onmessage, .onerror etc. events, which also has an event object rather than the message being passed as first parameter for message events. Expanding the event object to hold more information (including a flag for binary) is one thing I'd like to do. Adding addEventListener-support, as you propose, is another.

Would it be a fair middle way to introduce a proper event object for .onmessage and addEventListener('message', ...), but keep the current signature for the on('message', ...) callback? That should ensure that most (if not all) current users suffer no breaking changes.

@3rd-Eden
Copy link
Member

3rd-Eden commented Feb 6, 2012

This is mostly browser vs node talk. Which API do you want to adopt and support?

I would personally just keep the .addEventListener as a regular node.js event emitter and not some odd browser based API which wraps data in silly objects.

And just use the .onmessage as a browser compatibly layer. So you have clear separation between the two different API's and not an odd mix of both

@aslakhellesoy
Copy link
Contributor Author

I like that idea. This is my understanding of what you are saying:

  • An on API which is simpler to use, more node-idiomatic, but not W3C compatible.
  • An addEventListener/onmessage API that is W3C compatible.

@einaros
Copy link
Contributor

einaros commented Feb 6, 2012

@aslakhellesoy, exactly. I don't see any problems with that. I'd welcome a pullreq, or alternatively look into it myself later this week.

@aslakhellesoy
Copy link
Contributor Author

@3rd-Eden I agree that the W3C API wrapping messages in an event object is a little cumbersome, but it is the standard, and making this library follow it makes it easier to write portable code.

I'm not a big fan of the onmessage API, because it doesn't allow multiple listeners, so having a addEventListener function that behaves like W3C WebSockets would be great. Node folks wouldn't use it anyway, they'd just use on.

IIRC, older node versions aliased on to addEventListener, but this is not the case now, so the APIs wouldn't conflict.

@aslakhellesoy
Copy link
Contributor Author

Correction - Node's on is aliased to addListener. In any case - no risk of conflicts.

http://nodejs.org/docs/latest/api/events.html#emitter.on

@ikhattab
Copy link

So should we remove onmessage from documntation also https://github.com/einaros/ws/blob/master/doc/ws.md#websocketonmessage ?

@3rd-Eden
Copy link
Member

@ikhattab Why would we remove it? The functionality still exists.

@ikhattab
Copy link

@3rd-Eden oops sorry, my mistake.

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

No branches or pull requests

4 participants