-
Notifications
You must be signed in to change notification settings - Fork 1.2k
FloodSub API discussion #530
Comments
Topic should be a Topic Descriptor:
This is DHT, it won't be able to perform this feature.
Why Node.js stream? We already have pull-streams everywhere. Also, if a data event is a message, then it should be an Object Stream, not just a regular stream, otherwise what you get won't always be a message. It is also important to say how to cancel a subscription in this model. You also need some kind of |
I disagree. We currently don't support any of the properties Topic Descriptor gives us. A string is what we have for go-ipfs and we should have the same api for both in order to avoid the mess we had previously due to different APIs.
So "no" is what you mean?
We don't have pull-streams on the public API as far as I can tell. For example, files.cat() returns a node stream, not pull-stream https://github.com/ipfs/js-ipfs/blob/master/src/core/components/files.js#L71. Can you clarify what the intention is re. streams? Are we going to expose only pull-streams wherever the public API returns a stream? I don't remember that being the decision but rather what happens now (use pull-streams internally in js-ipfs + its sub-modules) and expose a "normal" stream to the consumer. Is that not the case anymore or did I misunderstand?
Can you elaborate on this? I don't understand what you mean.
Agreed. Will update the docs accordingly.
FYI this is not implemented in js-ipfs-api yet. I'll see what can be done here. |
Even though PubSub was something built up quickly for the purpose of showing its potential, we should make sure that what gets shipped as the final product doesn't have holes, especially the ones we can easily see, those being: easy to spam if no way of controlling who is publish is added. @whyrusleeping @jbenet might want to weigh in here, but AFAIK, we do want Topic Descriptors.
Correct, js-ipfs won't do discovery, until js-libp2p-dht gets implemented (also goal of the quarter
We are supporting the previous Node.js Stream API to avoid breaking user interface, we do however expose pull-stream API too (e.g: https://github.com/ipfs/js-ipfs/blob/master/src/core/components/files.js#L91). With the previously identified problems present in Node.js Streams, it is beneficial to guide out user base to pull-streams and explain how to convert them when necessary. We are already doing, see for example: https://github.com/libp2p/js-libp2p-swarm#this-module-uses-pull-streams. What I meant with regards to the Object Stream, is that Node.js Streams have two modes, see: https://nodejs.org/api/stream.html#stream_types_of_streams |
Sorry @diasdavid I was a bit too blunt on my disagreement on the Topic Descriptor vs string issue. What I meant was that, as it currently stands, we don't have Topic Descriptors anywhere, so what we should do is to make sure we can a) use the current implementation as soon as possible and b) be future-ready. Both in mind, we should have the topic name as a string for now and in future support Topic Descriptors and have Re. streams, I would really like to keep to ONE type of stream we return across the library. If we return node streams atm, let's return node streams. If we want to support pull streams, then let's make sure that once we do, it's consistent everywhere (ie. add them to every stream-returning function at the same time). |
Where are we with the naming regards to pubsub vs. floodsub? I remember @diasdavid proposed to name it floodsub instead pubsub in order to avoid future problems. @diasdavid can you provide some more info what the reasoning behind it is? Are we going to provide multiple pubsub mechanisms in one library? |
And what do we do about Node Streams vs. Pull Streams? I remember us discussing elsewhere that we could have the pull streams API in other places something like |
I've updated the original message (spec) with the discussion here. Need feedback @diasdavid @gavinmcdermott @dignifiedquire @victorbjelkholm |
I've added the API as a spec on ipfs-inactive/interface-js-ipfs-core#101, let's move the discussion there :) |
js-ipfs should expose the (IPFS) pubsub API.
API
UPDATED on 10th November 2016 by @haadcode
Subscribe
ipfs.pubsub.subscribe(topic, options, callback)
topic
is type of string.In the future,
topic
can also be type ofTopicDescriptor
(https://github.com/libp2p/pubsub-notes/blob/master/flooding/flooding.proto#L23). However, for now, only strings are supported.options
is an object with subscription options.Currently the
options
object is empty, ie. no options are supported, but in the future we'll add any options that can be passed into this argument.callback
signature is(err, stream)
wherestream
is a Stream.stream
emitsdata
event upon new message to the specifiedtopic
.If no
callback
is passed, returns aPromise
that resolves to a Stream:Unsubscribe
ipfs.pubsub.unsubscribe(topic)
topic
is type of string.Publish
ipfs.pubsub.publish(topic, data)
topic
is type of string.data
can be aBuffer
or anything that converts to a buffer withnew Buffer(data)
.Usage
The text was updated successfully, but these errors were encountered: