Skip to content
This repository has been archived by the owner on Aug 11, 2021. It is now read-only.

Implementation of the new API #190

Merged
merged 27 commits into from
Mar 21, 2019
Merged

Implementation of the new API #190

merged 27 commits into from
Mar 21, 2019

Conversation

vmx
Copy link
Member

@vmx vmx commented Jan 28, 2019

This is a huge PR. I've tried to keep the commits self-contained. So I suggest that your review one commit after another instead of the full PR. I'm happy to change the individual commits as I think for so many changes and breaking APIs it's important to have a clean history.

It might even make sense to review the "users" of that new API first, to get a feeling how this API really feels like:

@ghost ghost assigned vmx Jan 28, 2019
@ghost ghost added the status/in-progress In progress label Jan 28, 2019
@codecov
Copy link

codecov bot commented Jan 28, 2019

Codecov Report

Merging #190 into master will decrease coverage by 7.53%.
The diff coverage is 86.8%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #190      +/-   ##
==========================================
- Coverage   93.51%   85.97%   -7.54%     
==========================================
  Files           1        2       +1     
  Lines         185      164      -21     
==========================================
- Hits          173      141      -32     
- Misses         12       23      +11
Impacted Files Coverage Δ
src/util.js 100% <100%> (ø)
src/index.js 84.45% <85.15%> (-9.06%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update af37805...4666a54. Read the comment docs.

Copy link
Member

@achingbrain achingbrain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing major here.

src/index.js Show resolved Hide resolved
src/index.js Show resolved Hide resolved
src/index.js Show resolved Hide resolved
src/index.js Outdated
}

_put (cid, node, callback) {
callback = callback || noop

waterfall([
(cb) => this._getFormat(cid.codec, cb),
async () => {
return this._getFormat(cid.codec)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this work? I would have though you'd need to do something like:

waterfall([
  async (cb) => {
    try {
      cb(null, await this._getFormat(cid.codec))
    } catch (err) {
      cb(err)
    }
  },
  ...
])

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The async module supports async/await style coding.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently this only works with non-transpiled async functions? "Using ES2017 async functions" -> https://caolan.github.io/async/ (can't deep link, sorry)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Does this mean that tests pass as we run webpack with different configurations for development and production? I even think we don't really run tests with the Browser build we publish.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to Moz async functions are supported everywhere except IE, and our contributing guidelines say which Node version we support but not which browser types/versions.

Aegir uses babel/preset-env which I guess means it'll use native async unless you use IE, which will get a polyfill and this will break.

@alanshaw @olizilla @lidel - what browsers we officially support?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the browser build we currently use this browser list:

>1%, last 2 versions, Firefox ESR, not ie < 11

Which contains mobile browsers that don't support async/await.

src/util.js Outdated Show resolved Hide resolved
test/ipld-git.js Outdated Show resolved Hide resolved
test/ipld-bitcoin.js Outdated Show resolved Hide resolved
test/ipld-dag-pb.js Outdated Show resolved Hide resolved
test/ipld-eth-block.js Outdated Show resolved Hide resolved
test/ipld-bitcoin.js Outdated Show resolved Hide resolved
@achingbrain
Copy link
Member

achingbrain commented Feb 1, 2019

Is there a blessed async/bluebird style library we're going to use for async/iterable manipulations? Most of the methods in util.js would be useful to other modules.

Copy link
Member Author

@vmx vmx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope I've addressed everything.

loadFormat (codec, callback) {
if (codec !== 'dag-cbor') return callback(new Error('unexpected codec'))
setTimeout(() => callback(null, dagCBOR))
async loadFormat (codec) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally look quite often into the tests of projects to get examples of the API. Hence I wanted to make it explicit that generally an async function is needed.

Copy link
Member

@alanshaw alanshaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a note that there's no mention of "fancy iterator" in the return types of the API doc.

Also please can we think of a better name than "fancy iterator"?

src/index.js Outdated
this.support.rm = (multicodec) => {
if (this.resolvers[multicodec]) {
delete this.resolvers[multicodec]
if (options.loadFormat === undefined) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably better to typeof options.loadFormat !== 'function' here

options = {}
this.resolvers[codec] = {
resolver: format.resolver,
util: format.util
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If not allow add multiple then return this for chaining?

const codec = multicodec.getCode(codecBuffer)
if (this.resolvers[codec]) {
const codecName = multicodec.print[codec]
throw new Error(`Resolver already exists for codec "${codecName}"`)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just observing that we're being strict on adding a new format but not for remove - I cannot add if already exists, but it's not a problem if I try to remove a format that doesn't exist...

Personally I'd probably remove this check, coding around this with try+addFormat+catch+removeFormat+addFormat is painful.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder how often you really want to replace an existing format. I see it as a gatekeeper to prevent hard to debug bugs, where accidentally some format is replaced.

src/index.js Outdated
// get block
// use local resolver
// update path value
this.bs.get(cid, (err, block) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use promisify so we don't have to mix callbacks and promises and then you don't have to return new Promise, you don't have to try/catch around awaits in order to call resolve and this function gets way shorter, easier to read and understand. e.g.

resolve (cid, path) {
  if (!CID.isCID(cid)) {
    throw new Error('`cid` argument must be a CID')
  }
  if (typeof path !== 'string') {
    throw new Error('`path` argument must be a string')
  }

  const next = async () => {
    // End iteration if there isn't a CID to follow anymore
    if (cid === null) {
      return { done: true }
    }

    const format = await this._getFormat(cid.codec)

    // get block
    // use local resolver
    // update path value
    const block = await promisify(this.bs.get)(cid)
    const result = await promisify(format.resolver.resolve)(block.data, path)

    // Prepare for the next iteration if there is a `remainderPath`
    path = result.remainderPath
    let value = result.value
    // NOTE vmx 2018-11-29: Not all IPLD Formats return links as
    // CIDs yet. Hence try to convert old style links to CIDs
    if (Object.keys(value).length === 1 && '/' in value) {
      value = new CID(value['/'])
    }

    cid = CID.isCID(value) ? value : null

    return {
      done: false,
      value: {
        remainderPath: path,
        value
      }
    }
  }

  return fancyIterator(next)
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Promisifying" things that way doesn't work. Here's a minimal example:

'use strict'

const promisify = require('util').promisify

const CID = require('cids')
const IpfsBlockService = require('ipfs-block-service')
const IpfsRepo = require('ipfs-repo')

const ipfsRepoPath = '/tmp/getpromisifiedblocks'
const repo = new IpfsRepo(ipfsRepoPath, { init: true })

repo.init({}, (error) => {
  if (error) {
    throw error
  }

  repo.open(async (error) => {
    if (error) {
      throw error
    }
    const blockService = new IpfsBlockService(repo)

    const cid = new CID('zdpuAvwqFGiEVdAQjymUUyJzJK5nuZwn2QKw3WSj1vDdoZmRX')
    const block = await promisify(blockService.get)(cid)
    // Works (well it throws the error we would expect
    //blockService.get(cid, (error, block) => {
    //  if (error) {
    //    throw error
    //  }
    //})
  })
})

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I would've had @alanshaw's sk1llz I would've known to just use const block = await promisify(blockService.get.bind(blockService))(cid)

src/index.js Outdated
return callback(err)

let blocks
const next = () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As before, just make this an async function and use promisify. Lets not mix callbacks and promisies.

src/index.js Outdated

const next = () => {
// End iteration if there are no more nodes to put
if (nodes.length === 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, if we allow iterables we can't assume they have a length property - you need to iterate over it to determine the length, and ideally combine the iteration over the input with the operation to create a DAG node. I was anticipating this to be implemented something like:

function put (nodes) {
  // Generator function that creates a generator - a special iterator
  const gen = async function * () {
    // We iterate over `nodes`
    // `for await` works with iterators AND async iterators so `nodes` can be
    // created asynchronously and canceled half way through if we like!
    for await (const node of nodes) {
      const cid = await serializeAndPut(node)
      yield cid // halt execution of this loop, returning the `cid` of this new node
    }
  }
  return gen()
}

src/index.js Outdated
callback(null, format)
})
const format = await this.loadFormat(codec)
this.resolvers[codec] = format
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addFormat we pick out resolver and util - can be consistent (or use addFormat?)

src/index.js Outdated
})
const format = await this.loadFormat(codec)
this.resolvers[codec] = format
return format
}

_put (cid, node, callback) {
callback = callback || noop

waterfall([
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

promisify please!

src/index.js Outdated
* @return {Object} = Returns the deserialized node
*/
async _deserialize (block) {
return new Promise((resolve, reject) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

promisify!

iterator.last = () => exports.last(iterator)
iterator.all = () => exports.all(iterator)
return iterator
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason this is a decorator in IPFSX is because generators have return and throw functions that are called automatically by a for await loop to allow the generator to clean up when execution is aborted/finished.

const iterator = {
  return () {
    console.log('return called')
    return { done: true }
  },
  next () {
    return { done: false, value: 1 }
  },
  [Symbol.iterator] () {
    return iterator
  }
}

async function main () {
  let i = 0
  for await (const value of iterator) {
    i++
    if (i === 3) break
    console.log(value)
  }
}

main()

// output:
// 1
// 1
// return called

If we create a custom async iterator here that isn't a generator then breaking out of a loop that uses it will not call return, all that will happen is that next will not be called again. I don't think this is a problem right now with the code in this PR...but there could be issues here e.g. fs.createReadStream with a transform that converts raw data into an IPLD node, passed to ipld.put, halting half way would never end the stream, could cause the whole of the file content to be buffered into memory.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With "being a decorator" you mean that you're extending/modifying the iterator you passed in? I previously had that until @achingbrain mentioned that it's bad style to modify things you pass in (I agree with that). But in this case it might make sense.

What if fancyiterator just implements a return and throw? I know it won't be general purpose but good enough for js-ipld, won't it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typically a decorator would wrap the instance being decorated instead of modifying it, but you could argue it's a side-effect of a type system that doesn't allow modification at runtime.

@vmx
Copy link
Member Author

vmx commented Feb 21, 2019

@alanshaw I think I addressed all your comments (btw: they were great, the code is much better now). I also added some information about the extended iterators that are returned by the API (see https://github.com/ipld/js-ipld/blob/f5fa4ceba7dc1f85ba771dce00b1679fc099d990/README.md#api).

"aegir": "^17.1.1",
"bitcoinjs-lib": "^4.0.2",
"aegir": "^18.0.3",
"async": "^2.6.1",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still needs async?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes for the tests. The tests should be refactored as there's a lot of duplicated code, but I'd do that at a later stage.

src/index.js Outdated
@@ -1,21 +1,16 @@
'use strict'

const promisify = require('util').promisify
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

js-ipfs uses promisify-es6 - it would be nice to not put another promisify implementation in the bundle.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could Node.js native things be polyfilled with something like promisify-es6?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind, it seems to be compatible, so it was easy enough to switch.

src/index.js Outdated
// NOTE vmx 2018-11-29: Not all IPLD Formats return links as
// CIDs yet. Hence try to convert old style links to CIDs
if (Object.keys(value).length === 1 && '/' in value) {
value = new CID(value['/'])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor point but this will throw if value['/'] is not a CID. Should we instead try/catch incase this is actually data that just happens to be an object with one property that is /?

yield {
remainderPath: path,
value
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀 this is HAWT 😆

if (!Array.isArray(cids)) {
return callback(new Error('Argument must be an array of CIDs'))
get (cids) {
if (!typical.isIterable(cids) || typical.isString(cids) ||
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typical.isIterable does not check for async iterable i.e. [Symbol.asyncIterator] https://github.com/75lb/typical/blob/cee19519fadf276ba190e9543ead7695ebb119dc/index.js#L192-L198

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, good catch!

const block = await promisify(this.bs.get.bind(this.bs))(cid)
const format = await this._getFormat(block.cid.codec)
const node = await promisify(format.util.deserialize)(block.data)
yield node
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think eventually we want to pass our cids to the blockstore:

for await (const block of this.bs.get(cids)) { /* ... */ }

This is so the blockstore can look at the iterator and (for example) use leveldb "batch" or optimise the number of calls to the DHT.

From there my concern is that we're deserializing blocks in series whereas before we were doing all this in parallel and that'll be a huge perf hit.

I don't know how to do this with async iterators but there must be a simple way...I'll get back to you...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Performance is not a concern for IPLD atm. It's not optimized at all yet.

let formatImpl

const generator = async function * () {
for await (const node of nodes) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is ok for now (new functionality) but in the future we need consume these in parallel like https://github.com/pull-stream/pull-paramap

return e
// Only follow links if recursion is intended
if (options.recursive) {
cid = await maybeRecurse(block, treePath)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is super complicated and difficult to follow. Why don't we just call something like this here?:

for await (const path of this.tree(cid, treePath, userOptions)) {
  yield path
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tree() function will change a lot once the IPLD Formats use a new API. For now for me it's only important that it works.

const serialized = await promisify(format.util.serialize)(node)
const block = new Block(serialized, cid)
await promisify(this.bs.put.bind(this.bs))(block)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is ok for now but the block store needs to take a iterator and return an iterator that yields the blocks you put in:

put (nodes) {
  return this.bs.put(serialize(nodes))
}

@vmx
Copy link
Member Author

vmx commented Feb 28, 2019

@alanshaw thanks for the thorough review. I've pushed new commits, all comments should be either addressed by those commits or my comments.

@vmx
Copy link
Member Author

vmx commented Mar 1, 2019

My PR on typical to add async iterator support was merged an released!

vmx added 8 commits March 6, 2019 10:20
BREAKING CHANGE: `resolve()` replaces parts of `get()`.

The API docs for it:

> Retrieves IPLD Nodes along the `path` that is rooted at `cid`.

 - `cid` (`CID`, required): the CID the resolving starts.
 - `path` (`IPLD Path`, required): the path that should be resolved.

Returns an async iterator of all the IPLD Nodes that were traversed during the path resolving. Every element is an object with these fields:
 - `remainderPath` (`string`): the part of the path that wasn’t resolved yet.
 - `value` (`*`): the value where the resolved path points to. If further traversing is possible, then the value is a CID object linking to another IPLD Node. If it was possible to fully resolve the path, `value` is the value the `path` points to. So if you need the CID of the IPLD Node you’re currently at, just take the `value` of the previously returned IPLD Node.
BREAKING CHANGE: your custom format loading function needs
to be an async now.

So the signature for `options.loadFormat` is no longer:

   function (codec, callback)

but

  async functiont (codec)
Instead of storing a codec by their name, use their code instead.

The tests are changed from `base1` to `blake2b-8` as codecs for different
bases are no longer part of multicodec, but are part of multibase now.

BREAKING CHANGE: The `codec` parameter in `options.loadFormat()` is a number

Instead of returnign the name of the codec as string, the codec code (a number)
is now returned.

So if you e.g. check within the function for a certain format, it changes from:

    async loadFormat (codec) {
      if (codec !== 'dag-cbor') …
    }

To:

    async loadFormat (codec) {
      if (codec !== multicodec.DAG_CBOR) …
    }
BREAKING CHANGE: The API of `put()` changes.

The API docs for it:

> Stores the given IPLD Nodes of a recognized IPLD Format.

 - `nodes` (`Iterable<Object>`): deserialized IPLD nodes that should be inserted.
 - `format` (`multicodec`, required): the multicodec of the format that IPLD Node should be encoded in.
 - `options` is applied to any of the `nodes` and is an object with the following properties:
   - `hashAlg` (`multicodec`, default: hash algorithm of the given multicodec): the hashing algorithm that is used to calculate the CID.
   - `cidVersion` (`boolean`, default: 1): the CID version to use.
   - `onlyHash` (`boolean`, default: false): if true the serialized form of the IPLD Node will not be passed to the underlying block store.

Returns an async iterator with the CIDs of the serialized IPLD Nodes.
BREAKING CHANGE: `get()` is replacing the `getMany()` function.

The API docs for it:

> Retrieve several IPLD Nodes at once.

 - `cids` (`Iterable<CID>`): the CIDs of the IPLD Nodes that should be retrieved.

Returns an async iterator with the IPLD Nodes that correspond to the given `cids`.

Throws an error if a IPLD Node can’t be retrieved.
BREAKING CHANGE: `remove()` has a new API.

The API docs for it:

> Remove IPLD Nodes by the given `cids`

 - `cids` (`Iterable<CID>`): the CIDs of the IPLD Nodes that should be
  removed.

Throws an error if any of the Blocks can’t be removed. This operation is
*not* atomic, some Blocks might have already been removed.
BREAKING CHANGE: They replace the `support.add()` and `support.rm()` functions.

The API docs for it:

`.addFormat(ipldFormatImplementation)`:

> Add support for an IPLD Format

 - `ipldFormatImplementation` (`IPLD Format`, required): the implementation of an IPLD Format.

`.removeFormat(codec)`:

> Remove support for an IPLD Format

 - `codec` (`multicodec`, required): the codec of the IPLD Format to remove.
BREAKING CHANGE: This replaces the `treeStream()` function.

The API docs for it:

> Returns all the paths that can be resolved into.

 - `cid` (`CID`, required): the CID to get the paths from.
 - `path` (`IPLD Path`, default: ''): the path to start to retrieve the other paths from.
 - `options`:
   - `recursive` (`bool`, default: false): whether to get the paths recursively or not. `false` resolves only the paths of the given CID.

Returns an async iterator of all the paths (as Strings) you could resolve into.
vmx added 15 commits March 6, 2019 10:21
This way you can chain addFormat()/removeFormat() calls.
Instead of mixing callbacks and Promises, use `promisify()`.
Instead of mixing callbacks and Promises, use `promisify()`.
Instead of mixing callbacks and Promises, use `promisify()`.
Instead of mixing callbacks and Promises, use `promisify()`.
Instead of mixing callbacks and Promises, use `promisify()`.
Instead of manually coding an iterator, use an ES2015 generator.
Instead of manually coding an iterator, use an ES2015 generator.
Instead of manually coding an iterator, use an ES2015 generator.
Instead of manually coding an iterator, use an ES2015 generator.
Instead of manually coding an iterator, use an ES2015 generator.
js-ipfs is using the promisify-es6 module, so it makes sense that
we use it as well.
We are still supporting old style links, i.e. JSON where `/` as key
signals a link. Though the JSON could could also contain such a key
without having a CID as value. Instead of throwing an error, treat it
as not being a link.
More recent versions of typical detect async itertators as an iterable.
// get block
// use local resolver
// update path value
const block = await promisify(this.bs.get.bind(this.bs))(cid)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not do the promisification of the blockstore methods in the constructor instead of repeatedly during loops (e.g. here, during .get, .remove, .tree, etc)?

e.g.

class IPLDResolver {
  constructor (opts) {
    this.bs = {
       get: promisify(opts.bs.get.bind(opts.bs)),
       put: promisify(opts.bs.put.bind(opts.bs)),
       delete: promisify(opts.bs.delete.bind(opts.bs)),
       // ...etc
    }

    // ...etc
  }
}

Alternatively use something like Bluebird.promisifyAll until the lower level libraries are promise-aware?

@vmx
Copy link
Member Author

vmx commented Mar 18, 2019

Pushed a new commit that adds the *Many() functions to match the most recent #191. @alanshaw it would be great if you could have a look.


resolver.bs._repo.blocks.has(cid2, cb)
}
(node, cb) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is looking awkward within a waterfall, maybe it should be converted to an async test?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests were just adapted with minimal changes. The tests are in a bad shape currently and need a bigger refactor (there's lots of duplicated code). But I'd like to do it after this change is merged.

expect(sameAsNode.data).to.deep.equal(node.data)
return remove()

async function remove () {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are these wrapped up in their own function? the two lines should probably just be at the top level of this test

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests were just adapted with minimal changes. The tests are in a bad shape currently and need a bigger refactor (there's lots of duplicated code). But I'd like to do it after this change is merged.

src/index.js Outdated
* @param {number} format - The multicodec of the format that IPLD Node should be encoded in.
* @param {Object} [userOptions] - Options is an object with the following properties.
* @param {number} [userOtions.hashAlg=hash algorithm of the given multicodec] - The hashing algorithm that is used to calculate the CID.
* @param {number} [userOptions.cidVersion=1]`- The CID version to use.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

erroneous '`'

src/index.js Outdated
* @param {number} format - The multicodec of the format that IPLD Node should be encoded in.
* @param {Object} [userOptions] - Options are applied to any of the `nodes` and is an object with the following properties.
* @param {number} [userOtions.hashAlg=hash algorithm of the given multicodec] - The hashing algorithm that is used to calculate the CID.
* @param {number} [userOptions.cidVersion=1]`- The CID version to use.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

erroneous '`'

BREAKING CHANGE: put/get/remove functions are renamed

This commit introduces single item functions which are called `put()`/`get()`,`remove()`.

In order to put, get or remove multiple items you need to call
`putMany()`,`getMany()`/`removeMany()` now.
@vmx vmx merged commit 945fc61 into master Mar 21, 2019
@ghost ghost removed the status/in-progress In progress label Mar 21, 2019
@vmx vmx deleted the new-api-impl branch March 21, 2019 11:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants