-
Notifications
You must be signed in to change notification settings - Fork 26
Conversation
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.
Thanks for adding the JSDoc comments.
src/util.js
Outdated
* @returns {void} | ||
*/ | ||
exports.cid = (dagNode, options, callback) => { | ||
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/util.js
Outdated
} | ||
options = options || {} | ||
const hashAlg = options.hashAlg || resolver.defaultHashAlg | ||
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 doesn't work correctly. If options.version
is set to 0
, then version
would become 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?
The only error cases, that I can think of, are an invalid version or hash algorithm. Both cases should be handled by by underlaying software, |
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.
Good point. Often things are not handled correctly, but you are right, it should be handled by the underlying libraries. Could you have a quick look to see if they do handle it and if not open issues on those? If that's a too big ask, let me know and I'll check myself.
I'll also approve the other PRs where I mentioned the same thing.
Please squash merge (as mentioned on the other PRs :) |
I'll look into the libraries and report back. I'm |
I'm |
See ipld/interface-ipld-format#40
Closes #62