ipfs/js-ipfs

Exposed Interfaces (Callbacks & Promises, node.js streams and pull-streams)

Closed this issue · 13 comments

daviddias commented

This issue serves the purpose of continuing a conversation that several people have been having in different occasions, while opening so that anyone in the community can join.

Promises and Callbacks

Some time ago, we made the decision with the community involved to expose a Promises API at the top level of the JavaScript implementations of IPFS, more precisely: js-ipfs and js-ipfs-api

Discussion occurred (ipfs-inactive/js-ipfs-http-client#80) and now we have a Promise return in case of no Callback is passed. To be honest, it is not 'the worst' right now (for a while, if a error was thrown and the user passed a callback, the process would stale silently, that was a bug on promisify-es6 which is now solved), however it has some drawbacks, namely:

  • We can't do const stream = ipfs.get(hash), instead, we have to do ipfs.get(hash, (err, stream) => {}) or ipfs.get(hash).then((stream) => {}), which is annoying, especially when you are adding a level of indentation when it is not needed, because we can't return a promise and a stream in the same way we can have a callback and a stream.
  • We have to test every method twice, when in fact all that we do is wrapping the method on a promisify(). Not only "non promises" users have to take the promise shim, but also, IPFS developers have to do 2x the tests for the same codebase. Note, the same way that wrap our functions in a promisfy, any developer that really wants promises could do the same.

I'm a great fan of developer productivity and simplified documentation Nevertheless I do understand that there is a whole generation of Browser application developers that are super familiar with Promises and love to use them and are grateful for not having to wrap js-ipfs themselves.

Node.js Streams and Pull-Streams

We've learned that Node.js Streams have their quirks(#362) and we moved our internals to pull-streams, however, we won't break userspace. With this in mind, we will keep the same cat, add, get and so on API exposing "Node.js Streams" interfaces, with the proposal to add alongside a pull-stream interface, maybe namespaced by prefixing each method with pull, so that:

  • add -> pullAdd
  • get -> pullGet
  • cat -> pullCat
  • and so on

Thoughts?

Async/Await

We won't be considering Async/Await until it is wildly adopted and available, we want to avoid having to transpile code that gets required as a module, we already have been on that rabbit hole and we are pretty happy we got out of there. Right now js-ipfs supports Node.js 4 LTS.

RichardLitt commented

Thanks, @diasdavid! It's a bit hard for me to follow the flow of your discussion above. How would namespacing the various normal methods with pull* help with Promises and Callbacks? Are you suggesting we get rid of the wrapped Promises? How is Async/Await part of the picture?

daviddias commented

Oh, I should have made it more clear that these are pretty much 3 different discussions, but all part of the "exposed interfaces". So, answering your questions;

How would namespacing the various normal methods with pull* help with Promises and Callbacks?

Here, it would only mean that the Stream returned by the method is a pull-stream instead of a Node.js stream.

Today we have something like ipfs.files.createAddStream, which will have its pull counterpart ipfs.files.pullCreateAddStream

Are you suggesting we get rid of the wrapped Promises?

I just want to make sure everyone is conscious of the weight added by adding yet another async flow control, which might be that used or used by a small group of people that are used to wrap it themselves (majority of the modules only expose callbacks anyway)

How is Async/Await part of the picture?

async/await is promoted as the solution to the solution (promises) to the problem (callback). So to avoid entering in the twist of discussions about it, I just wanted to make it clear to everyone that we will only consider to support it when it is wildly adopted.

RichardLitt commented

Why not split these into three separate issues? Issues are cheap! And it would allow people to subscribe to the Async/Await thread, and to point to it, without conflating with other discussion.

Would a user still be able to request that a Node.js stream be returned? If not, I don't think this naming is useful. If there is a mix of streams returns, it certainly is, however.

Being conscious is good. Perhaps we should write this up in a doc? Long standing issues are not the best way to store information.

victorb commented

Promises and Callbacks

Yeah, after further thinking, I think we're not getting much benefit exposing both of the interfaces. It's hurting us in the way that we have to maintain two different interfaces while really only advocating for one of them. It would be easier for us to just support callbacks and let developers who want to use promises promisify their own things if needed.

Node.js Streams and Pull-Streams

Is there a reason we need to expose pull-streams? Have anyone asked for a pull-stream interface? Before exposing it, we should think about if it's something we really have to expose.

Async/Await

👍

parkan commented

I'm going to throw in my two cents for promises here. The native browser support at this point is nearly 100% and will not get any better (only non-supporting browsers are abandonware) and at least anecdotally the adoption rate is much higher than it was in 2015. Various repos in the IPFS and even some bits of js-libp2p code use them. I imagine most consumers of these APIs will have to wrap them -- we certainly do.

You also get async/await compatibility for free.

  • 👍2
  • ❤️1
parkan commented

(for reference, our js project is aleph)

mitra42 commented

David - can you comment on how the work at that link relates to Promises ? Its not clear to me, certainly we built around promises, and the one place we had to use promisify (Block.get) is the one place we see the code disappear - i.e. never return either the block or an error. .

daviddias commented

You will continue to have promises, that isn't going away.

I'm not sure if I understand what you mean by "code disappear"?

mitra42 commented

Sorry to be unclear David - what I mean is that its the only place where we see a call to the JS code never return, the Promise never resolves, and its unclear if this is the result of "promisify" or the underlying Block.get code (I think the latter).

daviddias commented

What you might be experiencing is that the IPFS node is looking for a block that still doesn't exist or that it is unreachable. We haven't implemented timeouts in JS (yet!) due to the complex nature of Canceable Requests.

I believe I have a good idea on how to implement them now, it is one of the top priorities. Read more at https://github.com/ipfs/interface-ipfs-core/issues/58#issuecomment-270093934

mitra42 commented

Great - because given the generally (and possibly inherent) unreliability of IPFS, its important to be able to handle errors. when you can't get the block rather than "sit and wait for nothing to happen" Reading your ipfs/interface-ipfs-core#58, it seems to me that the problem is less with cancelling ongoing requests for blocks, than with knowing when the block is not in the Dag and telling upper layers they can continue with alternatives.

Note ... I'm not sure about the diagnosis, we've been testing it on hashes that are known to exist (retrieve fine from https://ipfs.io/.... ) but fail on either block.get or files.cat depending on the hash,

I think we are off-topic, I should post as a separate issue.

daviddias commented

We have successfully updated the interface-ipfs-core spec to contemplate multiple options so that users can opt for Pull Streams, Readable Streams and also Callbacks/Promises if they are ok with not streaming content (ok for every case that doesn't require ingesting tons of data)

Current status:

Closing this issue, track the final step here #1086