-
Notifications
You must be signed in to change notification settings - Fork 37
feat: support passing AbortSignal instances to the configured blockservice #267
Conversation
Refs ipfs/js-ipfs#2884 |
This is so the user can signal that they are no longer interested in the results of the operation and system components can stop trying to fulfil it.
8ef58c4
to
c9fe11c
Compare
Codecov Report
@@ Coverage Diff @@
## master #267 +/- ##
==========================================
+ Coverage 86.84% 90.13% +3.28%
==========================================
Files 2 2
Lines 152 152
==========================================
+ Hits 132 137 +5
+ Misses 20 15 -5
Continue to review full report at Codecov.
|
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've just one question (inline).
This one seems to need ipfs/js-ipfs-block-service#89 to be released before it should be merged.
@@ -335,7 +348,7 @@ class IPLDResolver { | |||
if (treePaths.length === 0 && queue.length > 0) { | |||
({ cid, basePath } = queue.shift()) | |||
const format = await this._getFormat(cid.codec) | |||
block = await this.bs.get(cid) | |||
block = await this.bs.get(cid, 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.
Would it make sense to forward only the signal
value and not all of the options
object? This is really just a question.
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's a fair point, but the way we wrap modules inside modules, if we let the wrapping module be the gatekeeper of what gets passed on, it becomes tied to the API of that module more strongly.
This may cause problems down the line as we're potentially passing more arguments to the wrapped module than strictly necessary, but we trade off against flexibility in that case.
c9fe11c
to
5661753
Compare
The test failure seems to be an actual one. I can reproduce it locally even with a 30s timeout. I did run it locally with:
It doesn't happen all the time, but almost on every run I do. @achingbrain can you please have a look? |
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.
Taking back my approval as tests fail.
5661753
to
6488b8a
Compare
Looks better now. Did you force-push just to trigger a build? Cos you can do that from the Travis CI without changing public history.. |
I force-pushed because I did a rebase. |
This is so the user can signal that they are no longer interested in the results of the operation and system components can stop trying to fulfil it.