benjaminws / stomp-js

Implementation of the STOMP protocol in node.js
BSD 3-Clause "New" or "Revised" License
87 stars 47 forks source link

Please change EventEmitter inheritance way #25

Closed udavka closed 11 years ago

udavka commented 11 years ago

Benjamin, you should use

Stomp.prototype.__proto__ = process.EventEmitter.prototype;

instead of

Stomp.prototype = new process.EventEmitter();

to proper processing of several simultaneous client connections.

In your case any event is emitted to all clients instead of the single one.

benjaminws commented 11 years ago

Cool, I didn't know about this, thanks for the heads up. Let's chat about it a bit, so I can make sure I understand.

Initializing a new EventEmitter instance will emit the events to all clients connected, instead of just a single client connection? What you're proposing is that we just copy the prototype, which will ensure that we're not spamming all the clients connected with messages they don't care about. Does that sound right?

What about this: http://nodejs.org/api/util.html#util_util_inherits_constructor_superconstructor

It feels more idiomatic to me, and seems to do the same thing you're proposing. Am I missing something else in your proposal that this wouldn't solve?

benjaminws commented 11 years ago

Any thoughts on this before I implement it?

amwicfai commented 11 years ago

In my project, use stomp.js like this:

queues.forEach(function(queue) {
    initConnect(queue);
});

function initConnect(queue) {
    queue.mqClient = new Client.Stomp(stomp_args);
    queue.mqClient.connect();
        queue.mqClient.on('connected', function() {
          //event exception is here 
        });
}

thanks @benjaminws for your stomp.js thanks @udavkoid for your report. I use your code fixed and work fine.

benjaminws commented 11 years ago

Looks like this was taken care of by #26 in the way I had proposed, based on the docs I read.

Thanks for your work, and bringing this issue to my attention!