-
Notifications
You must be signed in to change notification settings - Fork 1.3k
refactor: make add only work on single items #3167
refactor: make add only work on single items #3167
Conversation
…ultiple `ipfs.add` returns single items only. Where a source would result in multiple items returned, only the last item is returned. `ipfs.addAll` retains the previous behaviour of `ipfs.add`. Examples: ```javascript const { cid, path, mode, mtime } = await ipfs.add('Hello world') ``` ```javascript const { cid } = await ipfs.add(Uint16Array.from([0, 1, 2])) ``` ```javascript const { cid } = await ipfs.add(fs.createReadStream('/path/to/file.txt')) ``` ```javascript const { cid } = await ipfs.add({ path: '/foo/bar/baz.txt', content: 'File content' }) // Creates a DAG with multiple levels of directories. // The returned cid is the CID for the root directory /foo // You can retrieve the file content with an IPFS path for await (const buf of ipfs.cat(`/ipfs/${cid}/bar/baz.txt`)) { ... } // Or get the CID of the nested file with ipfs.files.stat // (stat or something like it should really be a root level command) const { cid: fileCid } = await ipfs.files.stat(`/ipfs/${cid}/bar/baz.txt`) ``` BREAKING CHANGES: - `ipfs.add` only works on single items - a Uint8Array, a String, an AsyncIterable<Uint8Array> etc - `ipfs.addAll` works on multiple items
1c8247c
to
67f2b4e
Compare
…nly-work-on-single-items
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 have raised two things, but I'm leaving it up to you whether you think it's a right thing to do or not.
* `Uint8Array` | ||
* `FileObject` (see below for definition) | ||
* `Iterable<Uint8Array>` | ||
* `AsyncIterable<Uint8Array>` |
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 suggest to go as far as drop Iterable<Uint8Array>
and AsyncIterable<Uint8Array>
from the list. I think convenience added here is no where near to the complexity it adds.
Iterables can trivially be turned into blobs:
ipfs.add(new Blob(chunks))
And both async and sync iterables could also trivially be turned into FileObjects
ipfs.add({ content: chunks })
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.
We support AsyncIterable<Uint8Array>
in order to support adding node streams:
const fs = require('rs')
ipfs.add(fs.createReadStream('/path/to/file'))
|
||
module.exports = ({ addAll }) => { | ||
return async function add (source, options) { // eslint-disable-line require-await | ||
return last(addAll(source, 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.
I think just passing source
through isn't a best option here. I would much rather see addAll([source])
all the single file inputs should work and passing non singular inputs should error anyway. Am I overlooking something here ?
Better yet would be to throw an error if multi-file input is passed as:
- It can explain clearly that
addAll
should be used in place ofadd
. - Error will occur before work is done as opposed to doing a work and then producing unclear error saying result isn't iterable.
If Iterable<Uint8Array>
and AsyncIterable<Uint8Array>
as suggested in other comment, it would be both trivial to implement and remove all the ambiguity.
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.
Node streams throw a spanner in the works again. They are AsyncIterator<Uint8Array>
, so addAll([source])
becomes addAll(Iterable<AsyncIterable<Uint8Array>>)
which is not something we currently support. We could, I suppose, but so far no-one has asked for it.
I agree that add
should reject multiple items, but this can be done in a follow up PR.
@@ -9,7 +9,7 @@ const { withTimeoutOption } = require('../../utils') | |||
module.exports = ({ block, gcLock, preload, pin, options: constructorOptions }) => { | |||
const isShardingEnabled = constructorOptions.EXPERIMENTAL && constructorOptions.EXPERIMENTAL.sharding | |||
|
|||
return withTimeoutOption(async function * add (source, options) { | |||
return withTimeoutOption(async function * addAll (source, 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.
I would like to see a deprecation warning (via console.warn
) for source
s that represents a single file. So that we could remove support for that in future releases.
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.
It seems weird to me that a new API would warn but the existing API would error (from the comment above) as you're going to break existing code and only inconvenience new code.
I think just make them both error, but this can happen in a follow up PR.
…nly-work-on-single-items
…nly-work-on-single-items
`ipfs.add` now returns single items only. Where a source would result in multiple items returned, only the last item is returned. As a first pass `ipfs.add` still takes multiple items but only accepting single items is documented and considered supported. In the future it may throw if multiple items are passed. `ipfs.addAll` retains the previous behaviour of `ipfs.add`. Examples: ```javascript const { cid, path, mode, mtime } = await ipfs.add('Hello world') ``` ```javascript const { cid } = await ipfs.add(Uint8Array.from([0, 1, 2])) ``` ```javascript const { cid } = await ipfs.add(fs.createReadStream('/path/to/file.txt')) ``` ```javascript const { cid } = await ipfs.add({ path: '/foo/bar/baz.txt', content: 'File content' }) // Creates a DAG with multiple levels of directories. // The returned cid is the CID for the root directory /foo // You can retrieve the file content with an IPFS path for await (const buf of ipfs.cat(`/ipfs/${cid}/bar/baz.txt`)) { ... } // Or get the CID of the nested file with ipfs.files.stat const { cid: fileCid } = await ipfs.files.stat(`/ipfs/${cid}/bar/baz.txt`) // or ipfs.dag.resolve const { cid: fileCid } = await ipfs.dag.resolve(`/ipfs/${cid}/bar/baz.txt`) ``` ```javascript // To have `/foo` included in the ipfs path, wrap it in a directory: const { cid } = await ipfs.add({ path: '/foo/bar/baz.txt', content: 'File content' }, { wrapWithDirectory: true }) for await (const buf of ipfs.cat(`/ipfs/${cid}/foo/bar/baz.txt`)) { ... } ``` BREAKING CHANGES: - `ipfs.add` only works on single items - a Uint8Array, a String, an AsyncIterable<Uint8Array> etc - `ipfs.addAll` works on multiple items
ipfs.add
now returns single items only. Where a source would result in multiple items returned, only the last item is returned. As a first passipfs.add
still takes multiple items but only accepting single items is documented and considered supported. In the future it may throw if multiple items are passed.ipfs.addAll
retains the previous behaviour ofipfs.add
.Examples:
BREAKING CHANGES:
ipfs.add
only works on single items - a Uint8Array, a String, an AsyncIterable etcipfs.addAll
works on multiple items