API proposal for ipfs.files.add #20

hackergrrl · 2016-05-20T22:50:10Z

This is my first draft at merging the wildly different js-ipfs-api and js-ipfs add APIs. Notable things:

I dropped support for adding files from the local filesystem and for adding urls. I've written up a plan for making this change. These are nice utilities, but they don't belong in core.
Both object tuple formats (path, stream) and (path, content) are included here. It'd be nice if we could merge them into one style though. Maybe if content could be a Buffer or Readable?
I made the return object format consistent with what js-ipfs already does. This will require a change to js-ipfs or unixfs-engine.
We also accept a Buffer or a Readable stream.
If no argument is passed in, a Writeable stream is returned. I feel mixed about this. I understand the convenience of being able to pipe directly into ipfs.files.add(), but this style is confusing in the way it overloads the API call. I'm on the preference of dropping it but letting folks develop their own utilities for it if they're so inclined. This also gets ambiguous when I don't provide data or callback -- do I get back a Promise OR a Writeable? :D

ping @diasdavid @nginnever @dignifiedquire

haadcode · 2016-05-21T07:10:45Z

I would like the add to be:
ipfs.files.add('/tmp/myfile.txt') --> 'Qm...Foo'.

It'd be also fine to return a DAGNode, ie ipfs.files.add(path).toJSON().Hash --> 'Qm...Foo`.

If we want to give fine-grained options to define how/what the data is, being able to pass it as @noffle suggested sounds good.

ipfs.files.add(path) is consistent with go-ipfs CLI and does exactly what I would expect it to do
As a user, I shouldn't have to care how the file is read on the system level, I'm hooking up to a file system so it should function as such and abstract the system level functionality away from me
ipfs.files.addFiles(path) adds redundancy in the naming. It'd work better as ipfs.addFiles(path). Or perhaps as convenience method like: ipfs.files.addDirectory(path).
Currently ipfs.add(file) returns the full path of the file as an array of hashes and imo this is undesirable default behaviour. If a full path is needed, it should be an option, like recursive is now.

daviddias · 2016-05-21T10:47:30Z

README.md

+- a `Buffer` instance
+- a readable stream
+
+If no value `data` is provided, a Readable stream will be returned, which


s/Readable/Duplex

Oops -- I meant Writable. What's the use of this being a Duplex here?

You need to get back the DAGNodes correspondent to the files (and directories) that you are adding.

Ah right, it's a transform that's happening over the API! Got it. Thanks. :)

hackergrrl · 2016-05-21T16:43:17Z

Thanks for weighing in, @haadcode! Responses:

I would like the add to be:
ipfs.files.add('/tmp/myfile.txt') --> 'Qm...Foo'.

We're really keen on having IPFS Core only expose basic operations that all IPFS nodes will be able to support. Adding files from the local filesystem is something that e.g. the browser can't do, so I think it doesn't belong in core. That's not to say however that APIs on top couldn't add this functionality!

It'd be also fine to return a DAGNode, ie ipfs.files.add(path).toJSON().Hash --> 'Qm...Foo`.

Pinging @diasdavid for thoughts on returning first-class objects vs JSON -- I'm not sure what I think yet.

ipfs.files.add(path) is consistent with go-ipfs CLI and does exactly what I would expect it to do

I think we set up a bad expectation here: that CLI should be 1:1 with Core. CLI is a specific interface into the broader world of Core, since CLI has access to things like the local FS, pipes, a shell interpreter, etc. Because of this Core cannot (and shouldn't) match CLI's capabilities. In the case of ipfs adding a file, I think it makes more sense for the CLI to do the glue work of turning the files/dirs into streams and passing them into Core. I think @nginnever did this in unixfs-engine importer?

As a user, I shouldn't have to care how the file is read on the system level, I'm hooking up to a file system so it should function as such and abstract the system level functionality away from me

How should the API abstract this on the browser, though? Treat file paths as index-db paths? What about future IPFS nodes that have no storage like this at all?

ipfs.files.addFiles(path) adds redundancy in the naming. It'd work better as ipfs.addFiles(path). Or perhaps as convenience method like: ipfs.files.addDirectory(path).

Sure! I'm down for either. :)

Currently ipfs.add(file) returns the full path of the file as an array of hashes and imo this is undesirable default behaviour. If a full path is needed, it should be an option, like recursive is now.

Sorry, could you try to explain this again with some more context (maybe an example)? I'm not sure I understand.

daviddias · 2016-05-21T16:58:43Z

ipfs-core won't make reads (or writes) on the filesystem. Files are passed to ipfs-core, by application code or through the http-api, if using the daemon. This is the same pattern used in go-ipfs.

daviddias · 2016-05-21T17:00:42Z

README.md

+
+If no `callback` is passed, a promise is returned.
+
+


If no data is passed, then, a duplex stream is returned, that pretty much is the duplex stream returned by unixfs-engine Importer

What if no data is passed and no callback is passed? The possible return matrix starts to get messy -- you'd want a stream and a promise back. ;)

That would be a misuse of the API.

Note that we need the ability to return a duplex stream to be able to 'add files as they come', through the http-api or through the cli.

We can have the callback/promise provide a stream if no data is passed, but we can't have the function actually return a stream ever, since we went down the road of always returning promises.

Let's return the duplex stream on the callback on the interface-ipfs-core. This does not require changes on unixfs-engine. Sounds good?

daviddias · 2016-05-21T17:01:18Z

👍 for making the returned objects be first class DAGNodes :)

hackergrrl · 2016-05-22T20:36:02Z

Revisions applied -- just our convo re DAGNodes to resolve.

(Aside: I really wish we could simplify this API method: 4 possible input types, 2 types of return values, and both callback and promise control flows.)

daviddias · 2016-05-22T22:51:57Z

README.md

+error if the operation was not successful. If no value `data` is provided, `res`
+will be a Duplex stream, to which tuples like the above two object formats can
+be written and [DAGNode][] objects will be outputted. Otherwise, `res` will be
+an array of [DAGNode][]s.


Give an example please.

stream.write({path: <path to file>, stream: <readable stream>}) // write as many as you want stream.end() stream.on('data', function (fileDAGNode) { })

daviddias · 2016-05-22T22:52:38Z

Good progress :) Let's have tests :)

hackergrrl · 2016-05-24T02:39:29Z

js-ipfs: Make ipfs.files.add return DAGNodes ipfs/js-ipfs#261
js-ipfs-api: Make ipfs.files.add return DAGNodes. js-ipfs-http-client#281

haadcode · 2016-05-24T09:43:35Z

README.md

+be written and [DAGNode][] objects will be outputted. Otherwise, `res` will be
+an array of [DAGNode][]s.
+
+If no `callback` is passed, a promise is returned.


What does the Promise resolve to? An array of DAGNodes?

We should add examples for the returns

ipfs.files.add(path).then((res) => console.log(res)) // --> [DAGNode1, DAGNode2, ...] ???

haadcode · 2016-05-24T09:57:11Z

Good progress!

My thoughts atm:

I still think we really shouldn't limit the input arguments to an array of objects, each of the form.... We should be able to pass a path (or an array of paths). I understand the argument that the browsers don't have a file system but we also have the Node.js version and js-ipfs-api that both can support it and on browser, you can add files with the array of objects structure and have control over where the "file" data comes from. It's a lot more intuitive: ipfs, add some *files*, from this path as opposed to ipfs, add some *data*, that you should handle as files, and they come in like this....
I agree different types of result values is not ideal. However a callback is not a return value and as such should be fine if we always return a Promise and the callback's result argument is always a stream. This way the user has access to both: an actual result (array from the Promise) or a stream for further processing.
We should try to combine the input argument's object notation to contain only one type of object: { path: '/path', content: <buffer or stream> } and let the implementation handle the check which one it is.

daviddias · 2016-05-24T10:03:27Z

I still think we really shouldn't limit the input arguments to an array of objects, each of the form.... We should be able to pass a path (or an array of paths). I understand the argument that the browsers don't have a file system but we also have the Node.js version and js-ipfs-api that both can support it and on browser, you can add files with the array of objects structure and have control over where the "file" data comes from. It's a lot more intuitive: ipfs, add some files, from this path as opposed to ipfs, add some data, that you should handle as files, and they come in like this....

The result from the discussion we had (although I see not document) is that there will be, inside js-ipfs-api, extra API calls like:

ipfs.cli.add - which takes an input path, just like the CLI

These calls will only be available on the js-ipfs-api and are not part of the interface-core-spec

I agree different types of result values is not ideal. However a callback is not a return value and as such should be fine if we always return a Promise and the callback's result argument is always a stream. This way the user has access to both: an actual result (array from the Promise) or a stream for further processing.

Agree. However, it is fairly easy to distinguish between a call where data was already passed and a call where data wasn't (meaning that it wants the stream)

We should try to combine the input argument's object notation to contain only one type of object: { path: '/path', content: } and let the implementation handle the check which one it is.

Agree and I think this is what it is at the moment, did I miss something?

haadcode · 2016-05-24T10:31:42Z

ipfs.cli.add - which takes an input path, just like the CLI

Will this be available in Node.js version of js-ipfs? I think it should.

These calls will only be available on the js-ipfs-api and are not part of the interface-core-spec

I think the namespacing here is what makes it confusing for me. If we move the add under ipfs.cli.add and regardless have ipfs.files.add, people will use the latter as it makes sense intuitively whereas its input arguments imo don't. I think there might be a conflict of perspectives here: I'm looking at it from a higher-level, app developer perspective and perhaps what is proposed here is akin more to a library/module developer perspective. While both important, the top-level API needs to have as simple and intuitive UX as possible. The way I read it atm, with the proposed api, is that I'm adding arbitrary data to ipfs via the files api, not files. A file has a path, a file is located somewhere, so the intuitive thinking says I need to tell the api which file it is I want to add. Do you see what I mean?

Agree. The 'return value' (aka result) should be (and is) always the same, in this case. It should be the stream that will emit the several DAGNodes.

Wait, what's your understanding of the value the returned Promise resolves to? A stream or an array?

We should try to combine the input argument's object notation to contain only one type of object: { path: '/path', content: } and let the implementation handle the check which one it is.
Agree and I think this is what it is at the moment, did I miss something?

It's not reflected (yet?) in the PR, perhaps I'm looking at the wrong PR?

haadcode · 2016-05-24T10:48:04Z

Discussion today:

dignifiedquire> I think it would be better to have two different methods, than to try and shoehorn everything into one
daviddias> I'm also happy to have two different methods
dignifiedquire> just have a method `createAddStream`
dignifiedquire> then everybody knows what to expect
haad> was thinking the same.
daviddias> Ok, let's do that.
daviddias> Can someone type that while I'm at the subway? So that we don't loose this decision? :)

This sounds really good to me. Separate the stream and Promise control flows and it'll simplify everything nicely.

hackergrrl · 2016-05-24T16:32:43Z

Cool! Lots of thinking happened here while I was sleeping.

Yes: let's have functions that do one single thing than be a grab bag of inputs and outputs! Here's what I've aggregated from your comments:

break add into add and createAddStream (@dignifiedquire)
have one form of tuple input ({ path, content }) rather than supporting stream and content keys (@haadcode)

I've made these revisions here.

@haadcode and @diasdavid re ipfs.cli.add etc for file adding: I think the point of confusion here is on what interface-ipfs-core is providing. Here are two things this module is:

an interface for core ipfs functions
a way to use either a local JS node or a remote go/js node over HTTP with a single interface

Here is something it is not:

a drop-in replacement for all of the functionality in js-ipfs-api

It's unfortunate, but js-ipfs-api does more things than what "Core IPFS" does, so that overflow needs to go somewhere in its API.

So, and niceties that js-ipfs-api provides (like reading files from the FS) needs to be exposed from a higher level API than Core. Maybe this means a new module that builds atop interface-ipfs-core, like js-node-ipfs-helper that can do all of these local FS helper operations.

dignifiedquire · 2016-05-24T17:31:44Z

Re part 1: Yes that was what we meant

Re part 2:

I think it's a really good notion of having a module that wraps around both ipfs-core as well as js-ipfs-api to provide additional functionality, like interacting with the file system.

nginnever · 2016-05-25T00:48:16Z

test/files.js

+        path: `test-folder/${name}`,
+        content: fs.readFileSync(path.join(base, name))
+      })
+      const dirs = [


It would be nice if there was a test for an empty directory by adding one in data/test-folder/empty so that it can check this addition to js-ipfs pr.

Good idea. Delegating this to #23

daviddias · 2016-05-25T11:47:31Z

test/data/test-folder/ipfs-add.js

+  for (let i = 0; i < res.length; i++) {
+    console.log('added', res[i].Hash, res[i].Name)
+  }
+})


same as above

Do we still use this file?

daviddias · 2016-05-25T11:57:34Z

The tests should cover adding a files:

file with less than a block size
file bigger than a block size
file bigger than a block size, but that is not a multiple of the block size (size % 256KiB !== 0)
file that is 'big' (between 10 and 20Mb)
directory with files
directory with files and empty dirs
directory with other directories that have files and other empty dirs (nested dirs)

daviddias · 2016-05-25T11:59:48Z

Note for the future: @noffle open branches on the main repos, so that others can collaborate without having to PR your PR in order to make a change :)

hackergrrl · 2016-05-25T17:32:43Z

@diasdavid I filed #23 for the tests you mention. I'm going to focus on getting this code in before expanding the test suite.

hackergrrl · 2016-05-25T17:35:36Z

open branches on the main repos

I get used to the general github workflow, where you don't tend to have origin write access. 😀

hackergrrl · 2016-06-01T05:51:08Z

All feedback has been addressed!

@diasdavid:

merge this PR
npm version minor
npm publish

With this, I can update the dep in my js-ipfs and js-ipfs-api PRs 🎉

daviddias · 2016-06-01T11:23:12Z

Awesome work! SO CLOSE :D

It feels to me that postponing the tests that will check that this API operates as expect, might be a decision that will quickly backfire.

Anything against pushing this tests ahead? All of them are almost created over js-ipfs, js-ipfs-api and js-ipfs-unixfs-engine already, just need to be ported here.

daviddias · 2016-06-01T12:58:12Z

README.md

+```
+
+If no `callback` is passed, a promise is returned.
+


Add: example:

hackergrrl · 2016-06-01T17:44:58Z

@diasdavid done: these tests are all now present! All of them except for empty dirs were already present.

daviddias · 2016-06-01T18:49:16Z

👍 @noffle, rad :D

Going to jump into a plane down, wanna squash the commits?

Also replaces the old javascript test data with Project Gutenberg prose. Also returns object wrapping path+node on ipfs.files.add.

hackergrrl · 2016-06-01T20:51:39Z

Squashed and ready.

daviddias · 2016-06-03T08:23:11Z

test/files.js

+        const testfileBigPath = path.join(__dirname, './data/15mb.random')
+        testfileBig = fs.createReadStream(testfileBigPath, { bufferSize: 128 })
+      } else {
+        testfile = require('raw!./data/testfile.txt')


We now use the browserify transform to support fs.readFileSync on the transpilation process as well. We don't need to use the require('raw

hackergrrl · 2016-06-03T18:40:33Z

@diasdavid This is code I copied out of the existing repos, so these tests never ran in the browser previously either. Agreed 100% that we should make them do so, but given a) what work this PR is blocking, and b) that by not running in them in the browser we're no worse off than we were before (in fact, we're still better off, because there are most tests here than before!), I suggest we merge this now and come back to your those tests later. In fact, I've created a tracking issue for it. SGTM?

daviddias · 2016-06-05T16:26:29Z

Working on merging this. Made a branch on this repo so that we can all contribute to: #26 , closing this one.

daviddias reviewed May 21, 2016
View reviewed changes

daviddias reviewed May 22, 2016
View reviewed changes

hackergrrl force-pushed the ipfs-add branch from a298bd0 to 8e0f8f2 Compare May 24, 2016 02:42

haadcode reviewed May 24, 2016
View reviewed changes

This was referenced May 24, 2016

Rename "stream" to "content" in tuples. ipfs-inactive/js-ipfs-unixfs-engine#43

Merged

Make ipfs.files.add return DAGNodes. ipfs-inactive/js-ipfs-http-client#281

Closed

nginnever reviewed May 25, 2016
View reviewed changes

daviddias reviewed May 25, 2016
View reviewed changes

hackergrrl mentioned this pull request May 27, 2016

Make ipfs.files.add return DAGNodes ipfs/js-ipfs#261

Closed

2 tasks

daviddias reviewed Jun 1, 2016
View reviewed changes

README.md

```

If no `callback` is passed, a promise is returned.

Copy link

Contributor

daviddias Jun 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add: example:

hackergrrl force-pushed the ipfs-add branch from 8463441 to aaacdcb Compare June 1, 2016 16:17

hackergrrl mentioned this pull request Jun 1, 2016

ipfs.files.add tests #23

Closed

7 tasks

hackergrrl force-pushed the ipfs-add branch from a9d2e35 to 36b64a2 Compare June 1, 2016 17:46

hackergrrl added 2 commits June 1, 2016 13:48

Add ipfs.files.add documentation.

80edc88

Import ipfs.add tests from js-ipfs, js-ipfs-api.

3130b8f

Also replaces the old javascript test data with Project Gutenberg prose. Also returns object wrapping path+node on ipfs.files.add.

hackergrrl force-pushed the ipfs-add branch from 36b64a2 to 3130b8f Compare June 1, 2016 20:51

daviddias reviewed Jun 3, 2016
View reviewed changes

daviddias mentioned this pull request Jun 5, 2016

Interface for files.add #26

Merged

3 tasks

daviddias closed this Jun 5, 2016

API proposal for ipfs.files.add #20

API proposal for ipfs.files.add #20

Conversation

hackergrrl commented May 20, 2016 • edited Loading

haadcode commented May 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hackergrrl commented May 21, 2016

daviddias commented May 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddias May 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddias commented May 21, 2016

hackergrrl commented May 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddias commented May 22, 2016

hackergrrl commented May 24, 2016

Choose a reason for hiding this comment

haadcode commented May 24, 2016

daviddias commented May 24, 2016 • edited Loading

haadcode commented May 24, 2016

haadcode commented May 24, 2016

hackergrrl commented May 24, 2016 • edited Loading

dignifiedquire commented May 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddias commented May 25, 2016

daviddias commented May 25, 2016

hackergrrl commented May 25, 2016

hackergrrl commented May 25, 2016

hackergrrl commented Jun 1, 2016 • edited Loading

daviddias commented Jun 1, 2016

Choose a reason for hiding this comment

hackergrrl commented Jun 1, 2016 • edited Loading

daviddias commented Jun 1, 2016

hackergrrl commented Jun 1, 2016

Choose a reason for hiding this comment

hackergrrl commented Jun 3, 2016

daviddias commented Jun 5, 2016

hackergrrl commented May 20, 2016 •

edited

Loading

haadcode commented May 21, 2016 •

edited

Loading

daviddias May 21, 2016 •

edited

Loading

daviddias commented May 24, 2016 •

edited

Loading

hackergrrl commented May 24, 2016 •

edited

Loading

hackergrrl commented Jun 1, 2016 •

edited

Loading

hackergrrl commented Jun 1, 2016 •

edited

Loading