-
Notifications
You must be signed in to change notification settings - Fork 3
Conversation
src/index.js
Outdated
cid: (data, cb) => { | ||
cb(null, new CID(1, 'raw', data)) | ||
cid: (data, options, cb) => { | ||
if (options instanceof Function) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Searching through the existing IPFS/IPLD code base it seems that the preferred way for checking if something is a function is: if (typeof options !== 'function') {
. My guess is that it is related to code minifiers. Please change it.
src/index.js
Outdated
} | ||
options = options || {} | ||
const hashAlg = options.hashAlg || 'sha2-256' | ||
const version = options.version || 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not correct. If options.version
is set to 0
, it will result in version == 1
as 0
evaluates to false
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should've found that earlier. Can you please add tests for the error cases, when invalid options are passed in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm changing my mind (due to @richardschneider being right ipld/js-ipld-dag-cbor#66 (comment)).
When you merge, please do a "squash merge" and use the commit message of the first commit. That gives a clean Git history.
See ipld/interface-ipld-format#40