fix(dag): ensure `dag.put()` allows for optional options #801

0x-r4bbit · 2018-07-03T09:20:00Z

Note: Only review last commit as it's rebased on top of #785 to make test runner work

This is to align with API changes in

ipfs-inactive/interface-js-ipfs-core@011c417

and

Temporarily rebased on top of #785

Note: this is a WIP as I have to run now :D will make tests green once I'm back

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 <[email protected]>

daviddias · 2018-07-03T18:46:00Z

@alanshaw following your comment on #785 (comment), I've changed the base of this PR to your branch. If you approve, let's get this one merged there and then merge one clean PR to master.

0x-r4bbit · 2018-07-03T20:02:42Z

src/dag/put.js

@@ -15,29 +14,35 @@ module.exports = (send) => {

  return promisify((dagNode, options, callback) => {
    if (typeof options === 'function') {
-      return setImmediate(() => callback(new Error('no options were passed')))
+      callback = options
+    } else if (options.cid && (options.format || options.hash)) {


Notice that js-ipfs-api's implementation expects options.hash instead of options.hashAlg. This seems to be inconsistent with the spec as per https://github.com/ipfs/interface-ipfs-core/blob/master/SPEC/DAG.md#javascript---ipfsdagputdagnode-options-callback

Changing this would be a breaking change. @alanshaw how do you feel about this?

0x-r4bbit · 2018-07-03T20:04:43Z

src/dag/put.js

+    } else if (options.cid && (options.format || options.hash)) {
+      return callback(new Error('Can\'t put dag node. Please provide either `cid` OR `format` and `hash` options.'))
+    } else if ((options.format && !options.hash) || (!options.format && options.hash)) {
+      return callback(new Error('Can\'t put dag node. Please provide `format` AND `hash` options.'))


Turns out this is a breaking change as well (which is also one of the reasons CI fails at this time) Prior to this commit, it was okay to just have format with hash falling back to sha2-256.

Again, this would be a point of either introducing a breaking change and staying consistent with js-ipfs API, or leaving as is and live with the consequences of inconsistency.

@alanshaw your call.

License: MIT Signed-off-by: Alan Shaw <[email protected]>

alanshaw · 2018-07-04T09:03:46Z

@PascalPrecht I pushed a fix, I hope you don't mind, this PR is now on the critical path to getting the modular interface tests merged 😉

I think the problem was that hashAlg was being passed to dag.put but it was expecting hash. The API often allows you to pass the HTTP API querystring name instead of the interface-ipfs-core name for the argument (they differ sometimes) so I've preserved this functionality.

License: MIT Signed-off-by: Alan Shaw <[email protected]>

Sadly we can't guarantee ipfs.io will respond within 5s License: MIT Signed-off-by: Alan Shaw <[email protected]>

0x-r4bbit

@alanshaw I reviewed your changes and left a minor comment.

0x-r4bbit · 2018-07-04T09:46:21Z

test/util.spec.js

@@ -160,7 +160,9 @@ describe('.util', () => {
        .then(out => expectTimeout(ipfs.object.get(out[0].hash), 4000))
    })

-    it('with wrap-with-directory=true', (done) => {
+    it('with wrap-with-directory=true', function (done) {


I think the function keyword is a leftover here?

I'm using function here because on the next time we want to call this.timeout. Mocha provides an object with this method on it for the function context (this) but arrow functions do not allow you to change the context.

Got it. Thanks for clarifying!

daviddias

Looking good! :)

* feat: uses modular interface tests Reduces code repetition, allows test skipping and running only some tests. License: MIT Signed-off-by: Alan Shaw <[email protected]> * feat: skip unimplemented files tests and add ls* tests License: MIT Signed-off-by: Alan Shaw <[email protected]> * fix: adds skips for #339 License: MIT Signed-off-by: Alan Shaw <[email protected]> * fix: adds skips for key and miscellaneous License: MIT Signed-off-by: Alan Shaw <[email protected]> * feat: add types and util tests (skipped as currently failing) License: MIT Signed-off-by: Alan Shaw <[email protected]> * feat: add pin tests License: MIT Signed-off-by: Alan Shaw <[email protected]> * fix(pubsub): adds skips for tests on windows License: MIT Signed-off-by: Alan Shaw <[email protected]> * fix(config): adds skip for config.replace License: MIT Signed-off-by: Alan Shaw <[email protected]> * chore: re-adds bitswap tests License: MIT Signed-off-by: Alan Shaw <[email protected]> * chore: move detect-node back to dependencies License: MIT Signed-off-by: Alan Shaw <[email protected]> * chore: add skip reasons License: MIT Signed-off-by: Alan Shaw <[email protected]> * chore: update interface-ipfs-core dependency License: MIT Signed-off-by: Alan Shaw <[email protected]> * chore: add reason for pubsub in browser skips License: MIT Signed-off-by: Alan Shaw <[email protected]> * chore: add bitswap skips for offline errors License: MIT Signed-off-by: Alan Shaw <[email protected]> * fix: remove skip for test that was removed License: MIT Signed-off-by: Alan Shaw <[email protected]> * chore: update interface-ipfs-core version License: MIT Signed-off-by: Alan Shaw <[email protected]> * 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 <[email protected]> * fix(dag): fixes to allow options to be optional License: MIT Signed-off-by: Alan Shaw <[email protected]> * chore: update interface-ipfs-core dependency License: MIT Signed-off-by: Alan Shaw <[email protected]> * chore: update ipfsd-ctl dependency License: MIT Signed-off-by: Alan Shaw <[email protected]> * 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 <[email protected]> * 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 <[email protected]>

* feat: uses modular interface tests Reduces code repetition, allows test skipping and running only some tests. License: MIT Signed-off-by: Alan Shaw <[email protected]> * feat: skip unimplemented files tests and add ls* tests License: MIT Signed-off-by: Alan Shaw <[email protected]> * fix: adds skips for ipfs-inactive#339 License: MIT Signed-off-by: Alan Shaw <[email protected]> * fix: adds skips for key and miscellaneous License: MIT Signed-off-by: Alan Shaw <[email protected]> * feat: add types and util tests (skipped as currently failing) License: MIT Signed-off-by: Alan Shaw <[email protected]> * feat: add pin tests License: MIT Signed-off-by: Alan Shaw <[email protected]> * fix(pubsub): adds skips for tests on windows License: MIT Signed-off-by: Alan Shaw <[email protected]> * fix(config): adds skip for config.replace License: MIT Signed-off-by: Alan Shaw <[email protected]> * chore: re-adds bitswap tests License: MIT Signed-off-by: Alan Shaw <[email protected]> * chore: move detect-node back to dependencies License: MIT Signed-off-by: Alan Shaw <[email protected]> * chore: add skip reasons License: MIT Signed-off-by: Alan Shaw <[email protected]> * chore: update interface-ipfs-core dependency License: MIT Signed-off-by: Alan Shaw <[email protected]> * chore: add reason for pubsub in browser skips License: MIT Signed-off-by: Alan Shaw <[email protected]> * chore: add bitswap skips for offline errors License: MIT Signed-off-by: Alan Shaw <[email protected]> * fix: remove skip for test that was removed License: MIT Signed-off-by: Alan Shaw <[email protected]> * chore: update interface-ipfs-core version License: MIT Signed-off-by: Alan Shaw <[email protected]> * 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 <[email protected]> * fix(dag): fixes to allow options to be optional License: MIT Signed-off-by: Alan Shaw <[email protected]> * chore: update interface-ipfs-core dependency License: MIT Signed-off-by: Alan Shaw <[email protected]> * chore: update ipfsd-ctl dependency License: MIT Signed-off-by: Alan Shaw <[email protected]> * 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 <[email protected]> * 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 <[email protected]>

fix(dag): ensure dag.put() allows for optional options

78dd63c

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 <[email protected]>

0x-r4bbit mentioned this pull request Jul 3, 2018

fix(core/components/dag): make options in put API optional ipfs/js-ipfs#1415

Merged

alanshaw changed the title ~~fix(dag): ensure dag.put() allows for optional options~~ [WIP] fix(dag): ensure dag.put() allows for optional options Jul 3, 2018

alanshaw mentioned this pull request Jul 3, 2018

feat: modular interface tests #785

Merged

daviddias changed the base branch from master to feat/modular-interface-tests July 3, 2018 18:45

daviddias requested a review from alanshaw July 3, 2018 18:45

0x-r4bbit commented Jul 3, 2018

View reviewed changes

fix(dag): fixes to allow options to be optional

cc7a4f4

License: MIT Signed-off-by: Alan Shaw <[email protected]>

ghost assigned alanshaw Jul 4, 2018

ghost added the in progress label Jul 4, 2018

alanshaw added 3 commits July 4, 2018 10:07

chore: update interface-ipfs-core dependency

2ba55ac

License: MIT Signed-off-by: Alan Shaw <[email protected]>

chore: update ipfsd-ctl dependency

e3f4017

License: MIT Signed-off-by: Alan Shaw <[email protected]>

fix: increase timeout for addFromURL with wrap-with-directory

d678a27

Sadly we can't guarantee ipfs.io will respond within 5s License: MIT Signed-off-by: Alan Shaw <[email protected]>

0x-r4bbit commented Jul 4, 2018

View reviewed changes

daviddias approved these changes Jul 4, 2018

View reviewed changes

daviddias changed the title ~~[WIP] fix(dag): ensure dag.put() allows for optional options~~ fix(dag): ensure dag.put() allows for optional options Jul 4, 2018

daviddias merged commit c53d3cd into ipfs-inactive:feat/modular-interface-tests Jul 4, 2018

ghost removed the in progress label Jul 4, 2018

0x-r4bbit deleted the fix/dag-put-api branch July 4, 2018 10:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(dag): ensure `dag.put()` allows for optional options #801

fix(dag): ensure `dag.put()` allows for optional options #801

0x-r4bbit commented Jul 3, 2018

daviddias commented Jul 3, 2018

0x-r4bbit Jul 3, 2018 •

edited

Loading

0x-r4bbit Jul 3, 2018

alanshaw commented Jul 4, 2018

0x-r4bbit left a comment

0x-r4bbit Jul 4, 2018

alanshaw Jul 4, 2018

0x-r4bbit Jul 4, 2018

daviddias left a comment

fix(dag): ensure dag.put() allows for optional options #801

fix(dag): ensure dag.put() allows for optional options #801

Conversation

0x-r4bbit commented Jul 3, 2018

daviddias commented Jul 3, 2018

0x-r4bbit Jul 3, 2018 • edited Loading

Choose a reason for hiding this comment

0x-r4bbit Jul 3, 2018

Choose a reason for hiding this comment

alanshaw commented Jul 4, 2018

0x-r4bbit left a comment

Choose a reason for hiding this comment

0x-r4bbit Jul 4, 2018

Choose a reason for hiding this comment

alanshaw Jul 4, 2018

Choose a reason for hiding this comment

0x-r4bbit Jul 4, 2018

Choose a reason for hiding this comment

daviddias left a comment

Choose a reason for hiding this comment

fix(dag): ensure `dag.put()` allows for optional options #801

fix(dag): ensure `dag.put()` allows for optional options #801

0x-r4bbit Jul 3, 2018 •

edited

Loading