Skip to content

Comments

Implement .off#54

Open
narcisso wants to merge 3 commits intolouischatriot:masterfrom
narcisoguillen:master
Open

Implement .off#54
narcisso wants to merge 3 commits intolouischatriot:masterfrom
narcisoguillen:master

Conversation

@narcisso
Copy link

@narcisso narcisso commented Nov 1, 2018

Add .off and .unsubscribe

Given

 function handler(){}

We currently can subscribe a handler to a channel

nsp.on('channel', handler, function(){
});

This PR adds the ability to unsubscribe a handler from a channel

nsp.off('channel', handler, function(){
});

@RangerMauve
Copy link
Collaborator

I don't think it makes sense to have two methods to unsubscribe on a topic.

What about the current method doesn't work for you?

Also, the method you introduced has a memory leak in that the pool isn't being cleared when the client is closed, so the handlers are kept in memory forever.

@narcisso
Copy link
Author

narcisso commented Nov 5, 2018

as any other event based library I think it makes sense to unsubscribe a specific handler ( event ) from the registry, .off is an alias of .unsubscribe, If no specific event is send the whole topic will get unsubscribed.

There are scenarios where events ( handlers ) are dynamic and temporary, I've being part of complex real time micro service infrastructures and .off and .once come pretty handy

Thanks for the catch, I'll add a commit to clear the pool on end and quit

@RangerMauve
Copy link
Collaborator

This is a pretty breaking change, would you mind if we got more people to chime in that they want it before going forward? It'd also make sense to remove the existing unsubscribe functionality.

@RangerMauve
Copy link
Collaborator

Also, it'd be better to have the pool be a property under nsp since then you wouldn't have any global variables and it would be garbage collected along with the nsp instance.

@narcisso
Copy link
Author

narcisso commented Nov 5, 2018

Sure, lets have more minds on what makes more sense. I added it basically because I need it on a current app I'm working on.

Also I think we can split the code on methods ( if you think is a good idea ) like

  lib/
    index.js
    methods/
      on.js
      off.js
      once.js
    nsp/
      pool.js

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