Adding docs to hypercore-streams #8
Conversation
| @@ -63,6 +64,21 @@ class ReadStream extends Readable { | |||
| this.push(null) | |||
| return cb(null) | |||
| } | |||
| if (this.batch > 1) { | |||
There was a problem hiding this comment.
this needs to check if feed.getBatch exists also (remotehypercore does not have this atm)
There was a problem hiding this comment.
Would it be good to test the streams against remotehypercore as well? Or in other words: Is it desirable to consider remotehypercore as valid full implementation of the "hypercore-interface"?
There was a problem hiding this comment.
We already test it in hyperspace. This should only test against the "source of truth" which is hypercore :)
There was a problem hiding this comment.
How about a PR to remote-hypercore that adds getBatch?
| if (this.batch > 1) { | ||
| const batchStart = this.index | ||
| const batchEnd = Math.min(batchStart + this.batch, this.end, this.feed.length) | ||
| if (batchStart < batchEnd) { |
There was a problem hiding this comment.
i don't think this would ever trigger an else? ie, can we drop the if
There was a problem hiding this comment.
There could be an "undownload" operation or something alike happening inbetween. But testing for that is surely tricky.
There was a problem hiding this comment.
I don't see how that would trigger it still
There was a problem hiding this comment.
Okay, I looked into it a bit more. I am not sure I am able to phrase this correctly, but you can replicate my findings by throwing an error statement in a else block: } else { throw new Error('foo') } and then run the tests (this case is actually already covered by the tests case.
My understanding, formatted in a way that I could add it as comment to the code :)
// A batched live stream may start with an empty stream, resulting in both
// `batchEnd` and `batchStart` to be `0`. In this case a download needs to take place and
// we fall back to `get()` operation as it will trigger a download of the blocks linearly.
There was a problem hiding this comment.
this feels like it should be encapsulated with a IsBatchFinished or something that returns a bool, which you can call inside of if logic. Thank you for your contribution.
|
Thanks @martinheidegger, you probably just copied over the codebase so maybe @tinchoz49 knows about that 2nd q |
This PR adds documentation for the stream options to the documentation, as the stream options also describe "batch" I based this PR on #6. I will rebase this if necessary.
It also updates links to point to they
hypercore-protocolgithub org.