feat: Add reader to read files or part of files as streams #202

achingbrain · 2018-02-22T16:40:22Z

Adds a readable function to return a ReadableStream that supports reading arbitrary chunks of files with the signature readable(cid/path, ipldResolver, begin, end).

I made the begin, end arguments support the semantics of Array.slice(begin, end) as it's probably familiar to most JavaScript developers.

If this gets merged I'll do some follow up PRs for the specs repo & the js-ipfs examples.

One question I had - I've implemented this as a ReadableStream internally but I see a lot of pull streams in this module. Would it be better implemented internally as a pull stream and turned into a ReadableStream at a higher level similar to ipfs.files.catReadableStream()?

Related issues:

ipfs/js-ipfs#1231
ipfs/js-ipfs#348
#172

achingbrain · 2018-02-23T11:37:52Z

Actually forget that last paragraph, I refactored the PR to use pull streams.

I am getting a test error though, although it fails for me on master too, so unrelated to this PR:

  1) IPFS UnixFS Engine
       builder
         allows multihash hash algorithm to be specified:
     Error: multihash function 86 not yet supported
  2) IPFS UnixFS Engine
       builder
         allows multihash hash algorithm to be specified for big file:
     Error: multihash function 86 not yet supported

victorb · 2018-02-23T12:38:55Z

test/reader.js

+    })
+
+    it('reads bytes with a negative end', (done) => {
+      ipfs.files.add({


Everything except the assertion in the end seems duplicated across the test cases. Could you extract them into a function and call that instead please?

victorb · 2018-02-23T12:40:14Z

test/reader.js

+      pull(
+        reader('?!?', ipfs._ipldResolver),
+        collect((error) => {
+          expect(error.message).to.contain('Non-base58 character')


This error should also mention the value that was invalid, so the user can easily see what the wrong input is.

This error message comes from base-x so it could be improved there but it appears it's depended on in a few places so might be a more disruptive job than it appears.

daviddias · 2018-02-26T11:55:43Z

@pgte can you review this PR?

pgte

Everything looks super tight, great work on the tests.
Just left a question about the need of an extra dep.

pgte · 2018-02-27T18:10:55Z

src/reader/index.js

+  let streamPosition = 0
+
+  return pull(
+    asyncValues((cb) => {


Interesting, didn't know about pull-async-values.
Couldn't this be done instead with something like a pull.value([path]) and then using a chain of pull.map and pull.asyncMap?
I'm asking because this way we wouldn't need to add an extra dependency..

I couldn't find an easy way to asynchronously supply an array of values to pull-stream when you don't know the length of the values ahead of time using only the built-in functions.

The faq recommends using external modules for 1:many transforms and async steps so I'm not sure it's possible without adding an extra dep.

That said, the source for pull-async-values is pretty small - I can pull it in to this repo if required.

@achingbrain Thanks for explaining, it's fine as it is by me :)

daviddias · 2018-03-06T15:30:59Z

@achingbrain is this PR ready for any kind of Unixfs layout? I mean by that: balanced, flat and trickledag? Does it correctly know how to traverse the graph and avoid fetching unnecessary blocks when starting at a offset?

Also, weird that tests are failing with a multihash error. Mind checking into that?

achingbrain · 2018-03-11T08:14:08Z

is this PR ready for any kind of Unixfs layout?

I've refactored the tests to use the different UnixFS layouts, they also use an ipld instance directly now instead of spinning up an IPFS node so are faster and more consistent with other tests in this repo.

Does it correctly know how to traverse the graph and avoid fetching unnecessary blocks when starting at a offset?

Yes it uses the list of links to only fetch nodes that contain data from of the slice requested.

Also, weird that tests are failing with a multihash error. Mind checking into that?

This problem seems to have fixed itself 🤷 - I think the problem was in one of the dependencies, the test in question used the list of supported hashing algorithms exported by the multihashes module but it contained an algorithm that another module wasn't expecting or didn't know how to interpret.

mitra42 · 2018-03-11T21:16:20Z

I'm wondering how this will plug into a html

Part of the challenge is the parameters you get from the successive calls from the browser's media player. For example - I'm not sure what ipldresolver would be in this case ?

The approach I'm taking to the semantics is to have two calls:
async function(cid/path) which returns a function
function(opts) => stream - where opts is in the format { start, end } and returns a stream.

This works well with the RenderMedia module to build or edit

Note I posted a issue ipfs/js-ipfs#1258 where some of the videos that play in the gateway arent fetchable with files.cat, try testing with /ipfs/zdj7Wc9BBA2kar84oo8S6VotYc9PySAnmc8ji6kzKAFjqMxHS if you want to make sure you aren't hitting that issue.

achingbrain · 2018-03-20T09:58:17Z

@diasdavid @pgte @victorbjelkholm Can this be merged?

daviddias · 2018-03-20T17:32:51Z

Releasing it now. Thank you @achingbrain :D

achingbrain · 2018-03-21T18:13:48Z

N.b. superseded by #207

achingbrain force-pushed the add-readable branch 2 times, most recently from 1a1f526 to 942b21e Compare February 23, 2018 11:35

achingbrain force-pushed the add-readable branch from 942b21e to 0e4700f Compare February 23, 2018 11:41

victorb suggested changes Feb 23, 2018

View reviewed changes

achingbrain force-pushed the add-readable branch from 0e4700f to bf30dfc Compare February 23, 2018 22:31

daviddias added the status/ready Ready to be worked label Feb 26, 2018

daviddias requested a review from pgte February 26, 2018 11:55

pgte suggested changes Feb 27, 2018

View reviewed changes

pgte previously approved these changes Mar 2, 2018

View reviewed changes

achingbrain dismissed pgte’s stale review via 66be6b3 March 11, 2018 07:30

achingbrain force-pushed the add-readable branch from bf30dfc to 66be6b3 Compare March 11, 2018 07:30

achingbrain mentioned this pull request Mar 11, 2018

feat: Allow ipfs.files.cat to return slices of files ipfs/js-ipfs#1231

Merged

feat: Add reader to read files or part of files as streams

f82c509

achingbrain force-pushed the add-readable branch from 66be6b3 to f82c509 Compare March 20, 2018 15:27

ghost assigned achingbrain Mar 20, 2018

ghost added status/in-progress In progress and removed status/ready Ready to be worked labels Mar 20, 2018

daviddias approved these changes Mar 20, 2018

View reviewed changes

daviddias merged commit 833accf into ipfs-inactive:master Mar 20, 2018

ghost removed the status/in-progress In progress label Mar 20, 2018

achingbrain deleted the add-readable branch March 20, 2018 17:34

achingbrain mentioned this pull request Mar 21, 2018

Adds begin/end byte slices to exporter #207

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add reader to read files or part of files as streams #202

feat: Add reader to read files or part of files as streams #202

achingbrain commented Feb 22, 2018

achingbrain commented Feb 23, 2018

victorb Feb 23, 2018

achingbrain Feb 23, 2018

victorb Feb 23, 2018

achingbrain Feb 23, 2018

achingbrain Feb 23, 2018

daviddias commented Feb 26, 2018

pgte left a comment

pgte Feb 27, 2018

achingbrain Mar 2, 2018

pgte Mar 2, 2018

daviddias commented Mar 6, 2018 •

edited

Loading

achingbrain commented Mar 11, 2018

mitra42 commented Mar 11, 2018

achingbrain commented Mar 20, 2018

daviddias commented Mar 20, 2018

achingbrain commented Mar 21, 2018

feat: Add reader to read files or part of files as streams #202

feat: Add reader to read files or part of files as streams #202

Conversation

achingbrain commented Feb 22, 2018

achingbrain commented Feb 23, 2018

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 Feb 26, 2018

pgte left a 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 Mar 6, 2018 • edited Loading

achingbrain commented Mar 11, 2018

mitra42 commented Mar 11, 2018

achingbrain commented Mar 20, 2018

daviddias commented Mar 20, 2018

achingbrain commented Mar 21, 2018

daviddias commented Mar 6, 2018 •

edited

Loading