-
Notifications
You must be signed in to change notification settings - Fork 4
Adding docs to hypercore-streams #8
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,6 +42,7 @@ class ReadStream extends Readable { | |
this.tail = !!opts.tail | ||
this.index = this.start | ||
this.options = { wait: opts.wait !== false, ifAvailable: !!opts.ifAvailable, valueEncoding: opts.valueEncoding } | ||
this.batch = opts.batch || 1 | ||
} | ||
|
||
_open (cb) { | ||
|
@@ -63,6 +64,21 @@ class ReadStream extends Readable { | |
this.push(null) | ||
return cb(null) | ||
} | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i don't think this would ever trigger an else? ie, can we drop the if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see how that would trigger it still There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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: 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
this.index = batchEnd | ||
this.feed.getBatch(batchStart, batchEnd, this.options, (err, blocks) => { | ||
if (err) return cb(err) | ||
for (const block of blocks) { | ||
this.push(block) | ||
} | ||
cb(null) | ||
}) | ||
return | ||
} | ||
} | ||
this.feed.get(this.index++, this.options, (err, block) => { | ||
if (err) return cb(err) | ||
this.push(block) | ||
|
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
const ram = require('random-access-memory') | ||
|
||
module.exports = function () { | ||
const logByFilename = {} | ||
const factory = function (filename) { | ||
const memory = ram() | ||
const log = [] | ||
logByFilename[filename] = log | ||
return { | ||
read: logAndForward('read'), | ||
write: logAndForward('write'), | ||
del: logAndForward('del') | ||
} | ||
|
||
function logAndForward (op) { | ||
return function () { | ||
var statement = {} | ||
statement[op] = [].slice.apply(arguments) | ||
statement[op].pop() | ||
log.push(statement) | ||
return memory[op].apply(memory, arguments) | ||
} | ||
} | ||
} | ||
factory.log = logByFilename | ||
return factory | ||
} |
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.
this needs to check if feed.getBatch exists also (remotehypercore does not have this atm)
Uh oh!
There was an error while loading. Please reload this page.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about a PR to remote-hypercore that adds getBatch?