pubsub.unsubscribe without handler reference

[Isan-Rivkin]

Hi, continuing issue #436 here as a PR preparing. (Not ready yet)
I created a PR both on js-ipfs-http-client and js-libp2p as @alanshaw suggested.
If everything ok I will add tests to this repo as well.

Let me know what you think.

[alanshaw]

Ah, I thought you were suggesting that calling pubsub.unsubscribe without a handler would unsubscribe all. e.g. pubsub.unsubscribe('mytopic', err => {})

[jacobheun]

Ah, I thought you were suggesting that calling pubsub.unsubscribe without a handler would unsubscribe all. e.g. pubsub.unsubscribe('mytopic', err => {})

Me too. I would prefer to see unsubscribe just be overloaded and the documentation for it updated.

[Isan-Rivkin]

@alanshaw @jacobheun I thought so too, but the issue is that unsubscribe takes 3 parameters

• topic
• handler(msg)=>{}
• callback(err)=>{}
  callback is optional, so if you do overload and the caller passes 2 arguments how will you know which one is it? handler or callback?
  Since both of them are optional now.
  I would love to overload it if you have an idea and I'm missing something :-)

[jacobheun]

• callback is optional, so if you do overload and the caller passes 2 arguments how will you know which one is it? handler or callback?

Bleh, that's right. My initial thought is to use this as an opportunity to change the pubsub system over to async only, but having different system in different states would probably be unpleasant, although they technically already support promises. @alanshaw what do you think?

unsubscribeAll is probably the easiest approach if we don't change to async only. Otherwise callback would need to become not optional.

[Isan-Rivkin]

@jacobheun I can either stay with unsubscribeAll or make callback not optional.
The all-async options will probably take a little longer 0_0

[alanshaw]

Ah, good point.

I think this might have to wait for ipfs/js-ipfs#1670.

What do you think @jacobheun - I'm hesitant to add an additional API method for this...

[jacobheun]

I think doing this as part of the async iterators work is the better way to go. @alanshaw how do you feel about switching over individual subsystems (perhaps this is a conversation to have with the larger group)? We currently support promises for pubsub, so starting to remove support for callbacks system by system, and communicating those out, might be a reasonable approach. It would let us iterate on the APIs, and also allow users to update their components along with us, rather than large, significant refactors. Migration guides for each release would also be a bit easier to produce and consume.

[alanshaw]

In theory I'm pro this - it makes the process easier for us, but I think in this case we shouldn't do it. It makes it really difficult for users to know which release has which API and we also don't have an easy way to switch release versions in our API docs so it'll get confusing and we'll get a ton of issues saying "never calls callback" for people who upgrade, see callbacks working with some API methods but not others.

[Isan-Rivkin]

Just to clarify, this is on hold?

[jacobheun]

I think there is another option to get us the unsubscribeAll functionality without waiting. If unsubscribe only receives a single param, perform an unsubscribe all. Since callback is optional anyway, we could just say that if you want to unsubscribe all handlers for a topic, you must only include the topic name as a parameter.

// Will unsubscribe the handler for the given topic and call the callback if it's provided
ipfs.pubsub.unsubscribe(topic, handler, [callback])

// Will unsubscribe ALL handlers for the given topic
ipfs.pubsub.unsubscribe(topic)

Thoughts?

[Isan-Rivkin]

@jacobheun under current constraints it seems like a fair solution, im ok with adjusting the PR let me know 👍

[alanshaw]

SGTM

[jacobheun]

@Isan-Rivkin yes, let's move forward with the new approach.

[Isan-Rivkin]

this got accidentally closed because I merged this repo into my fork and they were aligned.

[Isan-Rivkin]

@jacobheun done, here and both on js-ipfs-http-client and js-libp2p
Not sure what to do about the codecov in js-ipfs-http-client?

[jacobheun]

Added some minor suggestions. Codecov in the http client will get resolved once this is released, as it uses the test suite from here.

[Isan-Rivkin]

@jacobheun Thanks for the responsiveness! I think resolved.

[Isan-Rivkin]

@alanshaw kindly pinging you for review :)

[Isan-Rivkin]

@alanshaw done, thanks! let me know if anything should be changed.

[Isan-Rivkin]

@alanshaw thanks for that review and that you provide examples! changed what you asked, let me know what you think