fix: make pubsub.unsubscribe async and alter pubsub.subscribe signature #260
Conversation
…s last args (promisify makes it a pain)
ghost
assigned 
|
Missing:
I want to land this before the next release of js-ipfs |
|
Ha, nice. I had to make |
SPEC/PUBSUB.md
Outdated
| - `options: Object`: Object containing the following properties: | ||
| - `topic: string` | ||
| - `discover: Boolean` - Will use the DHT to find other peers. | ||
| - `handler: (msg) => ()` - Event handler which will be called with a message object everytime one is received. The `msg` has the format `{from: string, seqno: Buffer, data: Buffer, topicIDs: Array<string>}`. |
There was a problem hiding this comment.
The handler moves into the options object? That's a bit of a weird pattern, much nicer when it was separated.
There was a problem hiding this comment.
I'm torn here. The thing is that promisify'ing things forces a weird arg check case.
There was a problem hiding this comment.
I agree with @victorbjelkholm on this one, I'd prefer that the implementation choices in js-ipfs didn't affect the public API.
i.e. I don't think promisify should dictate what our public API looks like even if it means handling some difficult argument combinations.
IMHO the most intuitive API would be this:
ipfs.pubsub.subscribe(topic, handler, options, callback)
ipfs.pubsub.unsubscribe(topic, handler, callback)Most important and non-optional arguments first, followed by optional args.
That said, if you just renamed the options arg to something like details then that would be better.
There was a problem hiding this comment.
I believe your proposal of:
ipfs.pubsub.subscribe(topic, handler, options, callback)
ipfs.pubsub.unsubscribe(topic, handler, callback)
Pretty much solves the main issue. I like it!
|
@alanshaw wanna take this API, spec and tests change from me? |
|
Yes |
License: MIT Signed-off-by: Alan Shaw <[email protected]>
License: MIT Signed-off-by: Alan Shaw <[email protected]>
License: MIT Signed-off-by: Alan Shaw <[email protected]>
License: MIT Signed-off-by: Alan Shaw <[email protected]>
License: MIT Signed-off-by: Alan Shaw <[email protected]>
License: MIT Signed-off-by: Alan Shaw <[email protected]>
alanshaw
changed the title
|
@diasdavid I think this and ipfs/js-ipfs#1348 and ipfs-inactive/js-ipfs-http-client#760 are good for a review now 🚀 |
|
@alanshaw feel empowered to merge and release this one :) (as minor). |
|
I am need 1 approving review |
Fix the flaky tests and API.