docs: major API changes

[vmx]

The whole IPLD APIs get a review to make them more consistent and
easier to use.

Changes to compared to the current spec:

• No more callbacks, everything is Promises.
• get():
  • Is renamed to resolve() to indicate that it also takes a mandatory
    path argument.
  • Doesn’t have a cid in the return value anymore. That use case is
    covered by returning all objects that were traversed. Their value
    will always be the CID of the next one to traverse. So if you want
    to know the CID of the current IPLD Node, just look at the value
    of the previously returned IPLD Node.
  • Doesn’t return a single value, but an iterator.
  • localResolve option is removed. If you want to resolve a single IPLD
    Node only, just stop the iterator after the first item.
• getStream() is removed without replacement. Use resolve() which uses
  async iterators instead.
• getMany() is renamed to get():
  • takes an iterable of CIDs as parameter.
• put():
  • takes an iterable of CIDs as parameter.
  • Doesn’t take cid as an option anymore. The CID is always calculated
    (Deprecate passing a CID to ipld.put? #175).
  • The options don’t take the format (which is a string), but the codec
    (which is a multicodec) (Deprecate passing a CID to ipld.put? #175).
  • the cidVersion option always defaults to 1.
• .treeStream() is renamed to .tree() and returns an async iterator.
• remove():
  • takes an iterable of CIDs as parameter.
• .support.add() is renamed to .addFormat().
• .support.rm() is renamed to .removeFormat().
• options.loadFormat() is no longer callback based but is using an async
  function.

Almost all open issues in regards to the IPLD API got adressed. One bigger
thing is the Patch API, which also isn’t part of the current specification.
Here’s an overview of the issues:

• IPLD Resolver patch API #66
  • IPLD Resolver patch API: There is no patch API yet
• IPLD resolver API changes #70
  • lazy value lookups: Can be done as IPLD Formats is pure JavaScript
  • get external / local paths only: @vmx doesn’t know what this is,
    considers it a “won’t fix”
  • toJSON + fromJSON - roundtrip: A roundtrip is not a goal anymore
    => won’t fix
  • put + patch api IPLD Resolver patch API #66: Patch API is still missing
  • text summary: @vmx doesn’t see a strong use case for this => “won’t fix”
  • globbing: Out of scope for js-ipld
• Deprecate passing a CID to ipld.put? #175
  • Deprecate passing a CID to ipld.put?: Current spec doesn’t allow
    put() with a CID
• API Review #182
  • API Review: Links to many other issues, I list the individual issues
    below
• getting the merkle proof with resolver.resolve #183
  • getting the merkle proof with resolver.resolve: resolve() returns
    all CIDs along the traversed path
• Pass down the options object to resolver.resolve() #184
  • Pass down the options object to resolver.resolve(): This needs a
    custom resolver. @vmx isn’t sure if the js-ipld API should make this
    possible, or of it should just be easy enough to create your own
    resolver.
• https://github.com/ipfs/interface-ipfs-core/issues/81
  • The dag API - One API to manipulate all the IPLD Format objects:
    Interesting discussion, but not relevant anymore.
• dag api: add {ls, tree} ipfs-inactive/interface-js-ipfs-core#121
  • dag api: add {ls, tree}: tree() is not part of js-ipld anymore as
    the IPLD Nodes are just JavaScript objects which you can easily traverse
    if you like. Though tree()-like functionality might be added as an
    example or separate module.
• dag get --localResolve vs dag resolve ipfs-inactive/interface-js-ipfs-core#125
  • dag get --localResolve vs dag resolve: This is solved by the new
    resolve() API
• add level option to ipfs.dag.tree ipfs-inactive/interface-js-ipfs-core#137
  • add level option to ipfs.dag.tree: tree() is not part of js-ipld
    anymore. It should be considered if tree() is implemented (probably
    as an example or separate module)

Closes #70, #175, #182, #183
Closes ipfs-inactive/interface-js-ipfs-core#121
Closes ipfs-inactive/interface-js-ipfs-core#125
Closed ipfs-inactive/interface-js-ipfs-core#137

[vmx]

This one is already reviewed an approved via #185. This will be merged once #190 is merged.

[lidel]

@vmx is this PR in sync with latest #190?

[vmx]

@lidel Yes. If not it's a bug.

[vmx]

@Gozala proposes a different (simpler) API: https://gozala.hashbase.io/posts/Constraints%20of%20an%20API%20design/

It really has good points and I think we should follow this route and change the API outlined here to the proposed one. As a start we won't implement any batching yet, but just do the single item commands.

What do you think @alanshaw @mikeal?

[achingbrain]

Only operating on individual CIDs/nodes means we can't perform optimisations at the library level - batching to not swamp the DHT, parallelising where it makes sense, etc. You consequently push that complexity onto the calling code - so the API might be simpler but the code you have to write to consume it in an efficient manner will not be.

Is it guaranteed that number of nodes match number of CIDs?

Since a node has a single CID, I'd say yes. Granted it's hard to guarantee this in code because iterators are unbounded.

Is order of nodes correspond to order of CIDs returned ?

Also yes, otherwise we're into non-deterministic territory. Again, hard to express in code but a documentation note should be sufficient?

If there is a failure to put nth node would rest of the nodes still be put or aborted ?

Our datastore is immutable so rolling back puts after a failure seems a little weird, but I guess there's the .remove method if the user wants that to happen.

I suppose you might want to control when and where to abort based on error conditions (e.g. abort after failure or continue after failure), but then you can drop down to the version without syntactic sugar:

const iter = ipld.put(someIterator)

while (true) {
  try {
    const { done, value } = await iter.next()
    // do something
  } catch (err) {
    // abort, continue, revert - it's up to you
  }
}

[achingbrain]

One thing:

put():

• takes an iterable of CIDs as parameter.

Shouldn't this be an iterable of nodes? Also, why not an async iterable so we don't have to hold the whole list in memory?

[vmx]

put():

• takes an iterable of CIDs as parameter.

Shouldn't this be an iterable of nodes? Also, why not an async iterable so we don't have to hold the whole list in memory?

It is nodes. The README is already right, the commit message is wrong. And it also can be an async iterable (the code has support for it).

[Gozala]

@achingbrain thanks for going through questions I’ve mentioned. As I later I attempted to point out those questions are symphtom of incidental complexity. I am huge proponent of batching optimization, but again if I were to distill that post into two centences - batching API needs acareful design, optimized towards it’s user rather than imposing solution optimized for internal batching on all users. Argument I’m trying to make that while (async) iterators seem neat they’re added complexity for non-batch operations and are far from good even for batched operations.

They might be adequate for single use case of streaming chunked ops and making that default interface for everyone is just makes all other uses prone to error.

In other words if batching is important use case let’s deaign separate API for that

[Gozala]

If there is a failure to put nth node would rest of the nodes still be put or aborted ?

Our datastore is immutable so rolling back puts after a failure seems a little weird, but I guess there's the .remove method if the user wants that to happen.

I think you're misunderstanding my question here, I was not talking about rolling back operations (which in theory could also fail) but rather that if you put([a, b, c]) and b fails would put still attempt to do c or not ?

I also know the answer, what I was trying to illustrate that those questions will arise and that is because in some use cases you'd want one behavior and in other the other.

[Gozala]

My proposal would be not to try and optimize API for batching without having support for that in first place. Once support for batching is being worked lets craft API that takes into account constraints and optimization opportunities into account as well as enables consumers to choose optimizations strategies, failure modes and some & mechanism to recovery (and maybe even unroll).

[mikeal]

I’m starting to get very confused about who the consumer of this API is. We’re making good improvements but as we start to narrow in on each one the expectations from the consumer of the API start to matter.

It seems like we’re trying to create a new, cleaner, layering scheme but we’re also shoving all of those new layers into a single API.

Right now, this single interface is responsible for:

• Encoding/decode data to/from objects to blocks.
• Resolving paths through multiple blocks.
• Storing and retrieving data from a pre-defined interface (BlockService).

If these were separate libraries/layers the interfaces between them would be much more obvious, but having them all together makes it unclear to me what the consumer wants to be doing. However, because the old interface combined all of this functionality it would be a much bigger shift to change it too much at a time when js-ipfs is also migrating to async/await.

If the goal here is just to have a replacement for the current interface that uses async/await so that we can continue to make progress on the migration in IPFS then let’s just merge and land this as soon as possible. I don’t think it’s productive to have larger meta conversations about the ideal interface while we also try to make the migration for js-ipfs as seamless as possible.

We can come back to better interfaces for IPLD generally after this lands.

[vmx]

If the goal here is just to have a replacement for the current interface that uses async/await so that we can continue to make progress on the migration in IPFS then let’s just merge and land this as soon as possible.

No it's more than this. It's a major clean-up and a major breaking change. As there are more projects that are using js-ipld than js-ipfs, it would be a bad if we would breaking the API a lot.

The current implementation of the new API quite trivial. We don't do any fancy optimizations, it's really all sequential.

Taking @Gozala concerns into account, the only change would be to make the get(), put() and remove() to use a single item instead of an iterable as input. Of course the consumers would need to be changed accordingly. But when I look e.g. at js-ipfs-mfs, it is using almost only the "single node" interface anyway (I won't link the others, but I expect it to be similar). So a simpler interface would even make code simpler.

So if we do this change now (chaning from "multi" to "single" operations), we can talk about all the batching/requesting several items at the same time later on, without breaking any of the API again.

[Gozala]

It is worth pointing out that current put / get API is unary, so if anything I’m proposing to not break that API without a really good reason

[achingbrain]

current put / get API is unary

This is not strictly true, there's .getMany(cids, callback) which IPFS makes use of and which we will lose if we switch to only one-in-one-out. The reason we use this is to not swamp the DHT (when it arrives in the next js-ipfs release).

when I look e.g. at js-ipfs-mfs, it is using almost only the "single node" interface

It does for fs manipulations (e.g. copying/moving files, etc) but when reading and writing whole files it uses the unixfs exporter/importer.

The exporter uses .getMany (as above) but there's no equivalent for writing multiple nodes simultaneously - it'd be a big win to be able to do so, certainly when using high-latency components like datastore-s3.

Using iterables for single-node operations is a bit clunky - can't we just have both?

interface IPLD {
  put<a>(node:a):Promise<CID<a>>;
  putMany<a>(nodes:AsyncIterable<a>):AsyncIterable<CID<a>>;
  get<a>(cid:CID<a>):Promise<a>;
  getMany<a>(cids:AsyncIterable<CID<a>>):AsyncIterable<a>;
}

In the case of errors, from an IPFS point of view it's best to fail quickly and give the user a useful error message so the default behaviour of exploding halfway through an iteration and stopping is fine.

If you want fancy error handling strategies you can just fall back to the non-syntactic sugar version as I suggested earlier. If the syntax offends, you can just wrap it in a module and publish it.

[vmx]

This is not strictly true, there's .getMany(cids, callback) which IPFS makes use of and which we will lose if we switch to only one-in-one-out. The reason we use this is to not swamp the DHT (when it arrives in the next js-ipfs release).

We also lose it in the current implementation I did for this API as it was too hard to do with the current callback based interface of the underlying libraries. So instead of implementing that properly I could invest my time on doing the batch API.

Using iterables for single-node operations is a bit clunky - can't we just have both?

One of my goals is keeping the API surface small. I don't think the clunky single node operation justifies adding new 3 new API calls (get, put, remove).

If you want fancy error handling strategies you can just fall back to the non-syntactic sugar version as I suggested earlier. If the syntax offends, you can just wrap it in a module and publish it.

One more question here. The current implementation would throw if there was an error. What would you do if you want to continue with the rest of the nodes? Would it be an option in that it returns errors instead of throwing them?

PS: @achingbrain It is not clear from my replies, but you've good points here.

[achingbrain]

We also lose it in the current implementation I did for this API

You mean that the implementation of getMany(cids)/get(cids) goes from parallel to serial? I think that's less of a concern for this conversation because we can fix the implementation without changing the interface, as long as the interface takes multiple CIDs in some form.

I don't think the clunky single node operation justifies adding new 3 new API calls (get, put, remove)

Not sure I agree - I think the improved DX makes it worthwhile.

The fact that the implementation adds non-standard .first, .last and .all methods to the returned generators shows you've felt the pain too..

What would you do if you want to continue with the rest of the nodes?

From an IPFS point of view It's hard to see why you'd want that. Imagine writing a 10TB file and the first node fails. You're not going to want to wait for the last node to be written to tell the user - Hey I'm done, by the way not all of your file made it kthxbye!

I suppose you could retry the failed nodes - if it became a requirement and there was no API available I'd just use the non-syntactic sugar version and do my own error handling.

[vmx]

The fact that the implementation adds non-standard .first, .last and .all methods to the returned generators shows you've felt the pain too..

Good point.

From an IPFS point of view It's hard to see why you'd want that.

I'm talking from an IPLD point of view here.

I suppose you could retry the failed nodes - if it became a requirement and there was no API available I'd just use the non-syntactic sugar version and do my own error handling.

I don't think you can resume an iterator that throws. Can you? Though you could manually keep track of the things you've already processed and then skip the one that failed. This is quite painful (as @Gozala explanations show), though I'm not sure how much of a problem that really is as it is certainly still possible somehow.

[achingbrain]

I don't think you can resume an iterator that throws. Can you?

Yes, if you don't use for await...of:

'use strict'

const iterator = {
  [Symbol.asyncIterator] () {
    return {
      i: -1,
      values: [0, 1, 2, 3, 4],
      next () {
        this.i++

        if (this.i === this.values.length) {
          return Promise.resolve({ done: true })
        }

        if (this.i === 2) {
          throw new Error('Nerp')
        }

        return Promise.resolve({ done: false, value: this.values[this.i] })
      }
    }
  }
}

async function surviveErrors () {
  console.info('--> Will continue after error')

  const iter = iterator[Symbol.asyncIterator]()

  while (true) {
    try {
      const { done, value } = await iter.next()

      console.info('done', done, 'value', value)

      if (done) {
        return
      }
    } catch (err) {
      console.error('got an error', err)
    }
  }
}

async function exitOnError () {
  console.info('--> Will not continue after error')

  try {
    for await (const value of iterator) {
      console.info('value', value)
    }
  } catch (err) {
    console.error('got an error', err)
  }
}

async function main () {
  await surviveErrors()
  await exitOnError()
}

main()

$ node test.js 
--> Will continue after error
done false value 0
done false value 1
got an error Error: Nerp
    at Object.next (/Users/alex/Desktop/test.js:16:17)
    at surviveErrors (/Users/alex/Desktop/test.js:32:42)
    at process._tickCallback (internal/process/next_tick.js:68:7)
    at Function.Module.runMain (internal/modules/cjs/loader.js:745:11)
    at startup (internal/bootstrap/node.js:283:19)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:743:3)
done false value 3
done false value 4
done true value undefined
--> Will not continue after error
value 0
value 1
got an error Error: Nerp
    at Object.next (/Users/alex/Desktop/test.js:16:17)
    at exitOnError (/Users/alex/Desktop/test.js:49:28)
    at process._tickCallback (internal/process/next_tick.js:68:7)
    at Function.Module.runMain (internal/modules/cjs/loader.js:745:11)
    at startup (internal/bootstrap/node.js:283:19)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:743:3)

[Gozala]

Using iterables for single-node operations is a bit clunky - can't we just have both?

I am very much advocating for having unary operations and having a separate batch operations.

One of my goals is keeping the API surface small. I don't think the clunky single node operation justifies adding new 3 new API calls (get, put, remove).

I think size of API surface is far less important than a good API.

[Gozala]

If you want fancy error handling strategies you can just fall back to the non-syntactic sugar version as I suggested earlier. If the syntax offends, you can just wrap it in a module and publish it.

For some reason there is a bias towards (async) iterators which I really don't understand. They introduce incidental complexity for both batch on unary operations and require user to keep track of additional local state. As I illustrated in the post you can have far better representation for batch operations that remove need for keeping track of state, pairing inputs with outputs and even can offer .retry() and .undo(). It is also certainly possible to add a method for async iterable consumption of results.

I would really like to know what are the basis for choosing async iterators ?

[Gozala]

This is quite painful (as @Gozala explanations show), though I'm not sure how much of a problem that really is as it is certainly still possible somehow.

Please consider that not every one is an expert and if you're API precludes that, that's a bad smell. Best APIs are the one that don't let you plug square peg into round hole

[vmx]

For some reason there is a bias towards (async) iterators which I really don't understand.

The reason is that at lot of IPFS/IPLD APIs are using pull-streams and async iterators are a good replacement for that.

[rvagg]

@vmx what's the strategy for moving forward on this? FWIW I agree with @Gozala's analysis, on top of which I'd add learnability and usability concerns. Consider the code and explanation overhead in the proto.school lessons. They'd be moving from teaching what (await ipld.get(cid)).value.foo breaks down as to what (await ipld.get([cid]).first()).value.foo does.

If there's concern about burdening downstream consumers (ipfs) with having to build their own abstractions to get iterable-like functionality, just bundle those use-cases in a utility module or as helper functions (or ideally as entirely separate packages) and leave the basic API clean and singular. let resultIter = require('ipld/util').getManyParallel(ipld, cidIter) and the list of helpers could expand to cover other use cases, fail-fast, skip-missing, order-undefined, strictly-sequential, whatever. The code you have in #190 could go straight into one of those.

[vmx]

My current plan would be to add single and multi versions. So we would have:

• get/getMany
• put/putMany
• remove/removeMany

This way, we have a simple interface, for single operations (like @Gozala suggested). We also need getMany() for js-ipfs so that we don't spam the DHT. So it would only be symmetric (and I like symmetric APIs) to also have "many" versions of the two other calls.

Implementation wise, the logic will be put into the single item versions and the "many"-versions will call into those. As a start it will be just sequential operations. But it leaves it open for the future to have more optimised "many"-calls.

How does that sound @achingbrain @alanshaw @Gozala @mikeal @rvagg?

[mikeal]

@vmx sounds good. I’ll note that, long term, we’ll still end up with more usage of async iterators than straightforward single methods because even simple mutations of a graph will produce multiple block changes.

[Gozala]

@vmx I think it's a good compromise. Longer term I would still encourage to design a batch operations tailored for specific use cases instead of generic async iterator.

[rvagg]

I still think that less is best for core APIs, particularly when they are trivial to build on top of outside of the core offering. The additional *Many assumes you know enough about all the eventual multiple-item use-cases (as @Gozala has pointed out). But since you all know more about the actual use-cases than I do so I'll defer to your better judgement!

[vmx]

I've updated the document. Please see the commit message as well as the new README. There's now also *Many() calls.

Can I please get an approval from @achingbrain @alanshaw @Gozala @mikeal @rvagg

[Gozala]

Probably node should be plural putMany(nodes, format, options)

[Gozala]

s/node/nodes/

[Gozala]

As I recall it was also accepted AsyncIterable<a> which is worth mentioning here if still the case.

[Gozala]

Worth specifying what happens if error occurs in the middle of the iteration.

[Gozala]

I think that it this is confusing way to communicate. I would suggest to just define a type / class e.g Batch and say that it implements async iteration interface along with methods first, last, all.

That would make communication clear:

1. It's clear that putMany takes async iterable, while it does return Batch, otherwise it leaves reader wondering if whether putMany should also be provided async iterable that has those extensions.
2. Batch could be exposed and could have Batch.from(asyncIterable) in case users need to create own API compatible instances without needing to do it on their own.
3. In the future Batch API can be further extended with other methods.

[vmx]

I've opened #196 as kind of a "catch all" issue for batch API related things.

[Gozala]

Should be getMany

[Gozala]

This is general question / feedback:

I find passing name / code of the codec counter intuitive & I think just providing a codec instance might be better for following reasons:

1. Type checkers / linters can catch mismatching / invalid references, but are useless with names.
2. If codec has not being installed you get an error, while with say dag.put(node, dag.codec[name]) you are encourage to see if format is known.
3. Don't necessarily need to install codec to use it.

I understand that without installing a codec / format decode will not work, however get could optionally take codec as well. In case of putMany / getMany it's less clear but maybe optional codec dict can be provided as in {[name]:Codec}.

Not a big deal, just in my code I find it more convenient to pass actual codecs than names.

[vmx]

I've opened #197 so that we don't lose that idea.

[Gozala]

Sidenote: I find tree to be a confusing name especially since it just returns paths. I think paths or list would have being far more clear.

I also kind of wish there was a way to traverse without following every branch .traverse(cid, path, {follow:path => boolean, order:path[] => path[] }) that way traverse(cid, path) is shallow, and .traverse(cid, path, {follow:Boolean}) is recursive, but it also provides an opportunity to avoid traversing irrelevant paths. As of order it can provide a way to choose traversal scheme e.g. depth-first could be as simple as returning path with most path components first.

[vmx]

Extracted into #198.

[vmx]

Thanks a lot @Gozala for the valuable comments. I opened a few issues based on your comments so that we discuss them outside this PR and hopefully get this one merged (together with the implementation) soon.