New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New Streaming and Buffered Interfaces #162

Merged
merged 27 commits into from Nov 17, 2017

Conversation

Projects
None yet
5 participants
@diasdavid
Member

diasdavid commented Oct 22, 2017

  • Files
    • Review and declare new function signatures
    • add
      • review spec
      • update tests
      • impl js-ipfs-api
      • impl js-ipfs
    • addReadableStream
      • review spec
      • update tests
      • impl js-ipfs-api
      • impl js-ipfs
    • addPullStream
      • review spec
      • create tests
      • impl js-ipfs-api
      • impl js-ipfs
    • cat
      • review spec
      • update tests
      • impl js-ipfs-api
      • impl js-ipfs
    • catReadableStream
      • review spec
      • create tests
      • impl js-ipfs-api
      • impl js-ipfs
    • catPullStream
      • review spec
      • create tests
      • impl js-ipfs-api
      • impl js-ipfs
    • get
      • review spec
      • update tests
      • impl js-ipfs-api
      • impl js-ipfs
    • getReadableStream
      • review spec
      • create tests
      • impl js-ipfs-api
      • impl js-ipfs
    • getPullStream
      • review spec
      • create tests
      • impl js-ipfs-api
      • impl js-ipfs
    • ls
      • review spec
      • update tests
      • impl js-ipfs-api
      • impl js-ipfs
    • lsReadableStream
      • review spec
      • create tests
      • impl js-ipfs-api
      • impl js-ipfs
    • lsPullStream
      • review spec
      • create tests
      • impl js-ipfs-api
      • impl js-ipfs
    • merge addFromFS to add will do in another PR
    • merge addFromUrl to add will do in another PR
> Add files and data to IPFS using a transform stream.
##### `Go` **WIP**
##### `JavaScript` - ipfs.files.createAddStream([options], [callback])
##### `JavaScript` - ipfs.files.AddReadableStream([options], [callback])

This comment has been minimized.

@dignifiedquire

dignifiedquire Oct 23, 2017

Member

please don't capitalize method names in JavaScript, that makes for some very surprising apis.

This comment has been minimized.

@diasdavid

diasdavid Oct 23, 2017

Member

ups, typo

@diasdavid diasdavid changed the title from WIP to New Streaming Interfaces Oct 30, 2017

@diasdavid diasdavid changed the title from New Streaming Interfaces to New Streaming and Buffered Interfaces Oct 30, 2017

@Beanow

Haven't had a moment to look at the tests yet, but some thoughts on the spec document.

- a [`Buffer instance`][b]
- a [`Readable Stream`][rs]
- a [`Pull Stream`][ps]
- a Path (caveat: will only work in Node.js)

This comment has been minimized.

@Beanow

Beanow Oct 31, 2017

If we're adding node only path strings, shouldn't we add browser only File instances? https://developer.mozilla.org/en-US/docs/Web/API/File

I think it would allow for great simplification and ample opportunity to do the right thing under the hood.

This comment has been minimized.

@diasdavid

diasdavid Oct 31, 2017

Member

Sounds good!

throw err
}
console.log(file.toString())

This comment has been minimized.

@Beanow

Beanow Oct 31, 2017

This toString call is tricky as it assumes utf8 encoding of the binary data. Perhaps a comment about encoding or passing utf8 explicitly would be a good hint.

console.log(file.path)
file.content.pipe(process.stdout)
console.log(file.path.toString())

This comment has been minimized.

@Beanow

Beanow Oct 31, 2017

Logging the path twice?
What about the path and filesize so we touched the content buffer as well.

This comment has been minimized.

@diasdavid

diasdavid Oct 31, 2017

Member

Thank you for catching that!

@@ -52,142 +57,342 @@ const files = [
]

This comment has been minimized.

@diasdavid

diasdavid Oct 31, 2017

Member

Really tempted to change the name of hash to cid.

@diasdavid

This comment has been minimized.

Member

diasdavid commented Nov 3, 2017

Spec and js-ipfs-api have been updated!

I've finished updating the spec and the implementation in js-ipfs-api. Next step is to implement the same interface in js-ipfs.

Please review as soon as possible to make sure that nothing gets missed. Overall I'm really happy with the simplicity of the new calls and I bet a lot more people will be able to use both modules without having to go through the learning of Node.js Streams and/or Pull Streams

@dignifiedquire

Only reviewed the interface, not the code

```JavaScript
const validCID = 'QmQ2r6iMNpky5f1m4cnm3Yqw8VSvjuKpTcK1X7dBR1LkJF'
const stream = ipfs.files.getReadableStream(validCID)

This comment has been minimized.

@pgte

pgte Nov 13, 2017

Shouldn't this be ... ipfs.lsReadableStream(.. instead?

#### `lsPullStream`
> Fetch a file or an entire directory tree from IPFS that is addressed by a valid IPFS Path. The files will be yielded as Readable Streams.

This comment has been minimized.

@pgte

pgte Nov 13, 2017

s/Readable Streams/pull stream ?

```JavaScript
const validCID = 'QmQ2r6iMNpky5f1m4cnm3Yqw8VSvjuKpTcK1X7dBR1LkJF'
const stream = ipfs.files.getReadableStream(validCID)

This comment has been minimized.

@pgte

pgte Nov 13, 2017

shouldn't this be ipfs.lsPullStream instead?

This comment has been minimized.

@diasdavid

diasdavid Nov 13, 2017

Member

thanks for the catch! :)

```JavaScript
const validCID = 'QmQ2r6iMNpky5f1m4cnm3Yqw8VSvjuKpTcK1X7dBR1LkJF'
ipfs.files.ls(validCID, function (err, files) {

This comment has been minimized.

@pgte

pgte Nov 13, 2017

shouldn't this be ipfs.ls instead?

@mitra42

This comment has been minimized.

mitra42 commented Nov 16, 2017

Does this fix the bug "c" from #164 which you commented was being worked on here?

@Beanow

This comment has been minimized.

Beanow commented Nov 16, 2017

Would this be what you're looking for?

https://github.com/ipfs/interface-ipfs-core/blob/dd922523e32ae1d99ed63d1bd18d4e5cb7a20323/SPEC/FILES.md#get

const validCID = 'QmQ2r6iMNpky5f1m4cnm3Yqw8VSvjuKpTcK1X7dBR1LkJF'

ipfs.files.get(validCID, function (err, files) {
  files.forEach((file) => {
    console.log(file.path)
    console.log(file.content.toString('utf8')) // <-- file.content is a Buffer
  })
})
@diasdavid

This comment has been minimized.

Member

diasdavid commented Nov 17, 2017

@mitra42 yes :)

diasdavid added some commits Nov 17, 2017

@diasdavid diasdavid merged commit 8f34b0e into master Nov 17, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@wafflebot wafflebot bot removed the in progress label Nov 17, 2017

@diasdavid

This comment has been minimized.

Member

diasdavid commented Nov 17, 2017

@diasdavid diasdavid deleted the revamp-stream-apis branch Nov 17, 2017

@mitra42

This comment has been minimized.

mitra42 commented Nov 17, 2017

@Beanow That case you've given is another case, i.e. where the hash refers to multiple files and having .content available would be a much nicer interface than having to use streams. Does that work if it was a single file. My two tests are with QmTds3bVoiM9pzfNJX6vT2ohxnezKPdaGHLd4Ptc4ACMLa (a file that should be 184324 bytes and therefore not sharded - and failed previous files.cat.
Qmbzs7jhkBZuVixhnM3J3QhMrL6bcAoSYiRPZrdoX3DhzB single sharded file which works in files.cat
And I also test with a simple json file uploaded with dag.put, which i think would break your files case as it will throw an "invalid node type" error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment