Skip to content
This repository has been archived by the owner on Dec 6, 2022. It is now read-only.

feat: new IPLD Format API #48

Merged
merged 1 commit into from
May 8, 2019
Merged

feat: new IPLD Format API #48

merged 1 commit into from
May 8, 2019

Conversation

vmx
Copy link
Member

@vmx vmx commented Apr 21, 2019

BREAKING CHANGE: The API is now async/await based

There are numerous changes, the most significant one is that the API
is no longer callback based, but it using async/await.

For the full new API please see the IPLD Formats spec.

@vmx vmx requested a review from rvagg April 21, 2019 23:34
@ghost ghost assigned vmx Apr 21, 2019
@ghost ghost added the status/in-progress In progress label Apr 21, 2019
@codecov
Copy link

codecov bot commented Apr 21, 2019

Codecov Report

Merging #48 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #48   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           3      3           
  Lines          84     65   -19     
=====================================
- Hits           84     65   -19
Impacted Files Coverage Δ
src/util.js 100% <100%> (ø) ⬆️
src/index.js 100% <100%> (ø) ⬆️
src/resolver.js 100% <100%> (ø) ⬆️

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 9dc7bb7...d9b3c93. Read the comment docs.

Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

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

seems ok, aside from some suggestions, insofar as my expertise on this goes

src/resolver.js Outdated
remainderPath: ''
})
}
exports = module.exports
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't be necessary, exports is a legacy of original commonjs and should always be defined and be the same object as module.exports

src/resolver.js Outdated
exports.resolve = async (binaryBlob, path) => {
let node = await util.deserialize(binaryBlob)

const parts = path.split('/').filter((x) => x)
Copy link
Member

Choose a reason for hiding this comment

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

filter(Boolean) is a simpler way of doing this

src/resolver.js Outdated
const traverse = function * (node, path) {
// Traverse only objects and arrays
if (Buffer.isBuffer(node) || CID.isCID(node) || typeof node === 'string' ||
node === undefined) {
Copy link
Member

Choose a reason for hiding this comment

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

maybe this should be node == null to account for null values too which will cause problems with Object.keys() below

src/resolver.js Outdated
const value = resolveField(dagNode, pathArray[0])
if (value === null) {
return callback(new Error('No such path'), null)
/*
Copy link
Member

Choose a reason for hiding this comment

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

are we ditching jsdocs in this? If so, maybe we could make some more moves away from aegir and it's massive dependency tree.

src/util.js Outdated
})
})

visibility.removeEnumerableProperties(deserialized, [
Copy link
Member

Choose a reason for hiding this comment

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

this seems to be the only use of visibility and it's fairly trivial, maybe it should be collapsed?

[ 'bits', 'merkleRoot', 'prevHash', 'transactions', 'witnessCommit' ].forEach((p) => {
  if (p in object) {
    Object.defineProperty(object, p, { enumerable: false })
  }
})

then visibility.js can be removed, there's a lot of noise in there that's not relevant

src/util.js Outdated
], callback)
const cid = async (binaryBlob, userOptions) => {
const defaultOptions = { cidVersion: 1, hashAlg: DEFAULT_HASH_ALG }
const options = mergeOptions(defaultOptions, userOptions)
Copy link
Member

Choose a reason for hiding this comment

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

Since the use of options here is so simple (no nested objects), Object.assign() will do this same work and you can remove the 'merge-options' dependency: const options = Object.assign({ cidVersion: 1, hashAlg: DEFAULT_HASH_ALG }, userOptions).

it('should return the timestamp', (done) => {
verifyPath(fixtureBlockHeader, 'timestamp', 1386981279, done)
it('should return the version', async () => {
await verifyPath(fixtureBlockHeader, 'version', 2)
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 return might be better than await in all of these, let it be resolved by mocha

done()
})
const verifyError = async (block, path, error) => {
await expect(
Copy link
Member

Choose a reason for hiding this comment

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

similar to above comment, this should probably just be return

test/util.spec.js Show resolved Hide resolved
BREAKING CHANGE: The API is now async/await based

There are numerous changes, the most significant one is that the API
is no longer callback based, but it using async/await.

For the full new API please see the [IPLD Formats spec].

[IPLD Formats spec]: https://github.com/ipld/interface-ipld-format
@vmx vmx merged commit 1a799aa into master May 8, 2019
@ghost ghost removed the status/in-progress In progress label May 8, 2019
@vmx vmx deleted the ipld-format-cleanup branch May 8, 2019 12:34
@vmx vmx mentioned this pull request May 8, 2019
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.

None yet

2 participants