node-redis-pubsub icon indicating copy to clipboard operation
node-redis-pubsub copied to clipboard

Implement .off

Open narcisso opened this issue 6 years ago • 5 comments

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(){
});

narcisso avatar Nov 01 '18 23:11 narcisso

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.

RangerMauve avatar Nov 05 '18 14:11 RangerMauve

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

narcisso avatar Nov 05 '18 17:11 narcisso

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 avatar Nov 05 '18 18:11 RangerMauve

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.

RangerMauve avatar Nov 05 '18 18:11 RangerMauve

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

narcisso avatar Nov 05 '18 19:11 narcisso