Interface for `ipfs.files.cat` (and alias `ipfs.cat`) #21

nginnever · 2016-05-21T19:07:22Z

This is the beginning of the cat spec. Some notable issues I can think of:

The test's aren't currently set up in npm package so I haven't tested the cat test file, but it is just a clone of the js-ipfs core test.
Not sure about the promises return. I have to understand promises a bit better or where they are currently being used. js-ipfs-api cat isn't set up with promises and the js-ipfs cli isn't either to my knowledge. cc @dignifiedquire

daviddias · 2016-05-21T21:51:24Z

The test's aren't currently set up in npm package so I haven't tested the cat test file, but it is just a clone of the js-ipfs core test.

Couldn't you try them with npm link?

Not sure about the promises return. I have to understand promises a bit better or where they are currently being used. js-ipfs-api cat isn't set up with promises and the js-ipfs cli isn't either to my knowledge. cc @dignifiedquire

Every js-ipfs-api call has a promise API since last October/November. However, the previous set up doesn't work for how we now have to implement this cross compatible methods, but we do have a way to achieve that, which can be observed in the already implemented Object API - https://github.com/ipfs/js-ipfs-api/blob/master/src/api/object.js

daviddias · 2016-05-21T21:51:34Z

README.md

@@ -55,6 +55,29 @@ test.all(common)

 A valid (read: that follows this interface) IPFS core implementation, must expose the following API.

+## Files
+
+### `Cat` 


daviddias · 2016-05-21T21:54:40Z

cat'ing a big file is important https://github.com/ipfs/js-ipfs-api/blob/master/test/api/cat.spec.js#L43-L58

daviddias · 2016-05-21T22:02:39Z

README.md

+- Buffer, the raw Buffer of the multihash (or of and encoded version)
+- String, the toString version of the multihash (or of an encoded version)
+
+`callback` must follow `function (err, stream) {}` signature, where `err` is an error if the operation was not successful and `stream` is a readable stream in object mode that contains objects representing a file in the form of { path: fileName, stream: fileData }.


In cat there is no filename, just the stream of the content.

"stream is a readable stream of the content referenced by the multihash."

The core is returning a readable object stream from the exporter, which will send one object containing { path: (the multihash provided to cat), stream: (a readable stream provided for that file) }

Should I PR a change to the js-ipfs core here https://github.com/ipfs/js-ipfs/blob/master/src/core/ipfs/files.js#L48

to this

const exportStream = Exporter(hash, self._dagS) exportStream.on('data', (file) => { callback(null, file.stream) })

yep. The path on cat is irrelevant, even misleading

daviddias · 2016-05-22T07:41:35Z

Is all of that test-data required?

nginnever · 2016-05-23T10:25:45Z

test/files.js

@@ -21,102 +22,94 @@ module.exports = (common) => {
      common.teardown(done)
    })

+    describe('.add', () => {


this fails on js-ipfs-api

TypeError: Invalid non-string/buffer chunk

nginnever · 2016-05-23T12:40:37Z

Having trouble writing tests that will work with both js-ipfs core and js-ipfs-api since the requirements vary between the two.

Conversation on how to respond from the add function in js-ipfs core here concluded with keeping dagnode objects as the return type for core.

{ path: 'data.txt',
    multihash: <Buffer 12 20 59 94 84 39 06 5f 29 61 9e f4 12 80 cb b9 32 be 52 c5 6d 99 c5 96 6b 65 e0 11 12 39 f0 98 bb ef>,
    size: 4,
    dataSize: 0 
}

While js-ipfs-api will expect JSON

{ Name: 'data.txt',
    Hash: 'QmVv4Wz46JaZJeH5PMV4LGbRiiMKEmszPYY3g6fjGnVXBS' 
}

Cat also can take both buffer and string forms of multihash for input in js-ipfs core, but in js-ipfs-api only string is supported.

Error: Invalid ipfs ref path

hackergrrl · 2016-05-23T17:01:59Z

README.md

+
+### `cat` 
+
+> Streams the data contained by an IPFS object(s) at the given IPFS multihash..


Is "object" accurate? Getting a stream of data from an ipfs object is object.get's job, right? These are "files"?

@diasdavid terminology help please

You are thinking right, @noffle. Object is a DAGNode, cat returns a File

daviddias · 2016-05-27T07:35:30Z

test/files.js

+        })
+      })
+
+      // This fails on js-ipfs-api


Why? Is there a PR with a fix?

that's an old comment from before i made this pr to upgrade the js-ipfs-api to accept buffer inputs Will remove :)

ipfs-inactive/js-ipfs-http-client#280

dignifiedquire · 2016-06-06T21:37:42Z

src/files.js

+          expect(res[0].path).to.equal('data.txt')
+          expect(res[0].node.size()).to.equal(17)
+          const mh = 'QmVv4Wz46JaZJeH5PMV4LGbRiiMKEmszPYY3g6fjGnVXBS'
+          expect(bs58.encode(res[0].node.multihash()).toString()).to.equal(mh)


using multihashes would be much nicer

dignifiedquire · 2016-06-06T21:40:44Z

lgtm woul just be nice to replace the b58 calls with multihashes

daviddias reviewed May 21, 2016
View reviewed changes

This was referenced May 21, 2016

updated cat core to return just file stream ipfs/js-ipfs#253

Closed

WIP cat interface core ipfs-inactive/js-ipfs-http-client#280

Closed

nginnever mentioned this pull request May 23, 2016

add http resource, files core upgrades, cat cli upgrades, begin interface-core integration hackergrrl/js-ipfs#1

Closed

11 tasks

nginnever reviewed May 23, 2016
View reviewed changes

nginnever mentioned this pull request May 23, 2016

add http resource, files core upgrades, cat cli upgrades, add/cat interface-core integration ipfs/js-ipfs#256

Closed

11 tasks

nginnever force-pushed the cat branch 2 times, most recently from eb03ac5 to 72018dc Compare May 23, 2016 12:20

hackergrrl reviewed May 23, 2016
View reviewed changes

nginnever force-pushed the cat branch from 30fd3a6 to 0a4c17b Compare May 25, 2016 18:15

daviddias reviewed May 27, 2016
View reviewed changes

nginnever force-pushed the cat branch 3 times, most recently from 4c337c2 to bc04eba Compare June 5, 2016 21:18

daviddias changed the title ~~API proposal for ipfs.files.cat~~ Interface for ipfs.files.cat (and alias ipfs.cat) Jun 6, 2016

cat

4fd85b6

daviddias force-pushed the cat branch from bc04eba to 4fd85b6 Compare June 6, 2016 19:19

dignifiedquire reviewed Jun 6, 2016
View reviewed changes

daviddias merged commit 42a3a55 into master Jun 6, 2016

nginnever mentioned this pull request Jun 6, 2016

Follow interface-ipfs-core for files.cat ipfs-inactive/js-ipfs-http-client#292

Merged

dignifiedquire deleted the cat branch June 7, 2016 07:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Interface for `ipfs.files.cat` (and alias `ipfs.cat`) #21

Interface for `ipfs.files.cat` (and alias `ipfs.cat`) #21

nginnever commented May 21, 2016

daviddias commented May 21, 2016

daviddias May 21, 2016

daviddias commented May 21, 2016

daviddias May 21, 2016

nginnever May 21, 2016

daviddias May 21, 2016

daviddias commented May 22, 2016

nginnever May 23, 2016

nginnever commented May 23, 2016

hackergrrl May 23, 2016

daviddias May 23, 2016

daviddias May 27, 2016

nginnever May 27, 2016 •

edited

Loading

dignifiedquire Jun 6, 2016

dignifiedquire commented Jun 6, 2016


		### `cat`

		> Streams the data contained by an IPFS object(s) at the given IPFS multihash..

Interface for ipfs.files.cat (and alias ipfs.cat) #21

Interface for ipfs.files.cat (and alias ipfs.cat) #21

Conversation

nginnever commented May 21, 2016

daviddias commented May 21, 2016

Choose a reason for hiding this comment

daviddias commented May 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddias commented May 22, 2016

Choose a reason for hiding this comment

nginnever commented May 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nginnever May 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dignifiedquire commented Jun 6, 2016

Interface for `ipfs.files.cat` (and alias `ipfs.cat`) #21

Interface for `ipfs.files.cat` (and alias `ipfs.cat`) #21

nginnever May 27, 2016 •

edited

Loading