fix(core/components/dag): make options in put API optional
#1415
Conversation
src/core/components/dag.js
Outdated
|
|
||
| const optionDefaults = { | ||
| format: 'dag-pb', | ||
| hashAlg: 'sha2-256' |
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.
@alanshaw you might wanna review those.
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.
These should be the same as go-ipfs (https://ipfs.io/docs/commands/#ipfs-dag-put) i.e. dag-cbor and sha2-256
Note also that cid can be passed here instead of format & hashAlg that you need to take into account. See https://github.com/ipfs/interface-ipfs-core/blob/master/SPEC/DAG.md#dagput
|
This is my first take on fixing #1395 I also wrote simple test for it in That's because as soon as Here's the error message I get: I took a closer look and https://github.com/ipfs/js-ipfs/blob/master/test/core/interface/bitswap.js#L35 @alanshaw maybe you have an idea what could cause this. I should add that this error does not occur when not linking |
|
Also, I wasn't sure about the commit message scope, let me know if that needs to be updated. |
|
@PascalPrecht PR #1389 updates To run new PR #1389 should be merging soon, so we'll just need to wait for that to go in before merging this PR. |
|
@travisperson thanks for clarifying! I thought the #1389 was only crucial to enable support for the API to skip testsuites as @alanshaw mentioned in #1395. Also I've just noticed that I'll rebase this PR on top of it then. Thanks again for clarifying! |
… options As discussed in ipfs/js-ipfs#1415 (comment)
7d83de4
to
705d73c
Compare
|
@travisperson @alanshaw Updated the PR accordingly. I've also created another PR on We might wanna merge that one, before this one can go in. |
|
Looks like CI is failing due to use of object spread operators. Will update the PR once again to make CI happy. |
705d73c
to
d5e88ed
Compare
|
Went with |
d5e88ed
to
03f6073
Compare
… options As discussed in ipfs/js-ipfs#1415 (comment)
|
CI is still failing, but those failing tests are unrelated to my changes. I've also noticed that this needs to be implemented in |
src/core/components/dag.js
Outdated
|
|
||
| const optionDefaults = { | ||
| format: 'dag-pb', | ||
| hashAlg: 'sha2-256' |
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.
These should be the same as go-ipfs (https://ipfs.io/docs/commands/#ipfs-dag-put) i.e. dag-cbor and sha2-256
Note also that cid can be passed here instead of format & hashAlg that you need to take into account. See https://github.com/ipfs/interface-ipfs-core/blob/master/SPEC/DAG.md#dagput
@alanshaw good point. The spec doesn't say anything about it, but I guess it'd make sense to throw an error in case someone uses the API wrong. Or would you prefer if |
03f6073
to
d0fdb67
Compare
I'm looking here: If the user passes a I'd also enforce Does that sound sensible? |
d0fdb67
to
08312b9
Compare
Correct! I was referring to whether the API should throw an error or not :)
Alright, gonna update the PR once again! |
08312b9
to
252ab1d
Compare
src/core/components/dag.js
Outdated
| } else if (options.cid && options.format && options.hashAlg) { | ||
| throw new Error('Can\'t put dag node. Please provide either `cid` OR `format` and `hashAlg` options.'); | ||
| } else if ((options.format && !options.hashAlg) || (!options.format && options.hashAlg)) { | ||
| throw new Error('Can\'t put dag node. Please provide `format` AND `hashAlg` options.'); |
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.
For a moment I was confused whether throwing synchronously inside promisify() makes sense, but I don't seem to have a handle to "reject" it either.
Looking at its testsuite, it seems to be the way to go: https://github.com/manuel-di-iorio/promisify-es6/blob/master/test.js#L79
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 would replace throw new ... with return callback(new ...)
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.
@richardschneider thanks for the hint! I assume that will automatically reject the promise?
I've updated the PR accordingly either way :)
252ab1d
to
5ff8281
Compare
… options As discussed in ipfs/js-ipfs#1415 (comment)
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 awesome 🚀 @PascalPrecht - thanks so much. I have just one small feedback point and we should be good to merge.
src/core/components/dag.js
Outdated
| @@ -9,6 +9,21 @@ const flattenDeep = require('lodash.flattendeep') | |||
| module.exports = function dag (self) { | |||
| return { | |||
| put: promisify((dagNode, options, callback) => { | |||
| if (typeof options === 'function') { | |||
| callback = options | |||
| } else if (options.cid && options.format && options.hashAlg) { | |||
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.
If we change this to if (options.cid && (options.format || options.hashAlg)) we'll catch cid+format, cid+hashAlg AND cid+format+hashAlg. At the moment if I provide one of hashAlg or format as well as cid it'll fall through to the next check which is for a different restriction.
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.
Oh yea, right. Much better. Let me update really quick.
src/core/components/dag.js
Outdated
| } else if (options.cid && options.format && options.hashAlg) { | ||
| throw new Error('Can\'t put dag node. Please provide either `cid` OR `format` and `hashAlg` options.') | ||
| } else if ((options.format && !options.hashAlg) || (!options.format && options.hashAlg)) { | ||
| throw new Error('Can\'t put dag node. Please provide `format` AND `hashAlg` options.') |
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.
Sorry, yes I completely missed that - you must return callback(new Error(...)) for these
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.
Done! Sorry I didn't come up with callback(new Error()) myself. It wasn't really clear, looking at the (minimal) docs and tests of promisify-es6.
This should be aligned now.
5ff8281
to
5ac8f05
Compare
|
Thanks @alanshaw I'm happy I managed to contribute to the IPFS project. Hope there's gonna be more opportunities to help out! :) |
The [dag.put](https://github.com/ipfs/interface-ipfs-core/blob/master/SPEC/DAG.md#dagput) interface takes an options object which is required, but should be optional with decent defaults. See dedicated test here: ipfs-inactive/interface-js-ipfs-core#316 This commit implements this behaviour. **Before**: ```js ipfs.dag.put(obj, options, (err, cid) => {...}); ``` ^ Prior to this commit, without passing `options`, this call resulted in an error. **After**: ```js ipfs.dag.put(obj, (err, cid) => {...}); ``` ^ This is now perfectly fine. Fixes ipfs#1395 License: MIT Signed-off-by: Pascal Precht <pascal.precht@gmail.com>
5ac8f05
to
5ff633e
Compare
|
FYI: saw that latest master landed #1389, rebased this one on top of it therefore again. |
BTW, that would be incredible ✨ |
|
Gotcha. WIll take care of that. |
* test(dag): introduce test that ensure `dag.put` can be called without options As discussed in ipfs/js-ipfs#1415 (comment) * chore(SPEC/DAG): update DAG spec to cover optional `options` argument License: MIT Signed-off-by: Pascal Precht <pascal.precht@gmail.com> * test: add test to ensure defaults are set License: MIT Signed-off-by: Alan Shaw <alan@tableflip.io>
License: MIT Signed-off-by: Alan Shaw <alan@tableflip.io>
This is to align with API changes made in ipfs-inactive/interface-js-ipfs-core@011c417 and ipfs/js-ipfs#1415 License: MIT Signed-off-by: Pascal Precht <pascal.precht@gmail.com>
@alanshaw please find a WIP PR here ipfs-inactive/js-ipfs-http-client#801 |
* fix(dag): ensure `dag.put()` allows for optional options This is to align with API changes made in ipfs-inactive/interface-js-ipfs-core@011c417 and ipfs/js-ipfs#1415 License: MIT Signed-off-by: Pascal Precht <pascal.precht@gmail.com> * fix(dag): fixes to allow options to be optional License: MIT Signed-off-by: Alan Shaw <alan@tableflip.io> * chore: update interface-ipfs-core dependency License: MIT Signed-off-by: Alan Shaw <alan@tableflip.io> * chore: update ipfsd-ctl dependency License: MIT Signed-off-by: Alan Shaw <alan@tableflip.io> * fix: increase timeout for addFromURL with wrap-with-directory Sadly we can't guarantee ipfs.io will respond within 5s License: MIT Signed-off-by: Alan Shaw <alan@tableflip.io>
* feat: uses modular interface tests
Reduces code repetition, allows test skipping and running only some tests.
License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
* feat: skip unimplemented files tests and add ls* tests
License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
* fix: adds skips for #339
License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
* fix: adds skips for key and miscellaneous
License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
* feat: add types and util tests (skipped as currently failing)
License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
* feat: add pin tests
License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
* fix(pubsub): adds skips for tests on windows
License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
* fix(config): adds skip for config.replace
License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
* chore: re-adds bitswap tests
License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
* chore: move detect-node back to dependencies
License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
* chore: add skip reasons
License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
* chore: update interface-ipfs-core dependency
License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
* chore: add reason for pubsub in browser skips
License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
* chore: add bitswap skips for offline errors
License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
* fix: remove skip for test that was removed
License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
* chore: update interface-ipfs-core version
License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
* chore: update deps and test on CI (#802)
* chore: update interface-ipfs-core
* fix(dag): `dag.put()` allows for optional options (#801)
* fix(dag): ensure `dag.put()` allows for optional options
This is to align with API changes made in
ipfs-inactive/interface-js-ipfs-core@011c417
and
ipfs/js-ipfs#1415
License: MIT
Signed-off-by: Pascal Precht <pascal.precht@gmail.com>
* fix(dag): fixes to allow options to be optional
License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
* chore: update interface-ipfs-core dependency
License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
* chore: update ipfsd-ctl dependency
License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
* fix: increase timeout for addFromURL with wrap-with-directory
Sadly we can't guarantee ipfs.io will respond within 5s
License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
* fix: skip bitswap offline tests on all platforms
The offline tests create and stop a node. ipfs/kubo#4078 seems to not only be restricted to windows.
License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
* feat: uses modular interface tests
Reduces code repetition, allows test skipping and running only some tests.
License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
* feat: skip unimplemented files tests and add ls* tests
License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
* fix: adds skips for ipfs-inactive#339
License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
* fix: adds skips for key and miscellaneous
License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
* feat: add types and util tests (skipped as currently failing)
License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
* feat: add pin tests
License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
* fix(pubsub): adds skips for tests on windows
License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
* fix(config): adds skip for config.replace
License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
* chore: re-adds bitswap tests
License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
* chore: move detect-node back to dependencies
License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
* chore: add skip reasons
License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
* chore: update interface-ipfs-core dependency
License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
* chore: add reason for pubsub in browser skips
License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
* chore: add bitswap skips for offline errors
License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
* fix: remove skip for test that was removed
License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
* chore: update interface-ipfs-core version
License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
* chore: update deps and test on CI (ipfs-inactive#802)
* chore: update interface-ipfs-core
* fix(dag): `dag.put()` allows for optional options (ipfs-inactive#801)
* fix(dag): ensure `dag.put()` allows for optional options
This is to align with API changes made in
ipfs-inactive/interface-js-ipfs-core@011c417
and
ipfs/js-ipfs#1415
License: MIT
Signed-off-by: Pascal Precht <pascal.precht@gmail.com>
* fix(dag): fixes to allow options to be optional
License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
* chore: update interface-ipfs-core dependency
License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
* chore: update ipfsd-ctl dependency
License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
* fix: increase timeout for addFromURL with wrap-with-directory
Sadly we can't guarantee ipfs.io will respond within 5s
License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
* fix: skip bitswap offline tests on all platforms
The offline tests create and stop a node. ipfs/kubo#4078 seems to not only be restricted to windows.
License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>

The dag.put interface
takes an options object which is required, but should be optional with
decent defaults.
See dedicated test here: ipfs-inactive/interface-js-ipfs-core#316
This commit implements this behaviour.
Before:
^ Prior to this commit, without passing
options, this call resulted in an error.After:
^ This is now perfectly fine.
Fixes #1395
License: MIT
Signed-off-by: Pascal Precht pascal.precht@gmail.com