-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Conversation
256bdce
to
8ea66f9
Compare
This PR changes the behavior of I'm not sure if this is the best approach and would be curious to know how Go handles it. Tagging @whyrusleeping and @Kubuxu for possible input. |
In go-ipfs we use an http trailer for errors
…On Tue, Oct 10, 2017, 11:21 AM Dmitriy Ryajov ***@***.***> wrote:
This PR changes the behavior of files.add considerably as it switches to
a chunked response (stream of objects) that reports progress. One issue
with this, is that the response is streamed and it's not possible to report
an error with a standard 500 code, since it's already been initiated with a
200 code. My current solution is to report an error as part of the stream
of objects and catch it in the ProgressStream through stream, if an error
object is detected ({Message: '', Code: #}) the stream emits the error,
which should propagate correctly.
I'm not sure if this is the best approach and would be curious to know how
Go handles it. Tagging @whyrusleeping <https://github.com/whyrusleeping>
and @Kubuxu <https://github.com/kubuxu> for possible input.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1036 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABL4HKF7r5crOSo_9b2W9T-wKZpG5fm7ks5sqykkgaJpZM4PzbFm>
.
|
@whyrusleeping ah! Didn't know about those - but makes sense! Thanks! |
Coverage has gone down due to https://github.com/ipfs/js-ipfs/pull/1036/files#diff-dcaf16a1df33e0e8f40195e527b9294dR244, but I don't see any tests that exercise that functionality directly. @diasdavid any ideas of how to tests this so that coverage goes up? |
prog(total) | ||
} | ||
|
||
options.progress = progress | ||
pull( | ||
pull.values(normalizeContent(data)), | ||
importer(self._ipldResolver, 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.
@diasdavid I noticed that there are two entry points for importer, here and above on line 26. The one above, gets called from the HTTP API, this one is called from the regular callback flow, they seem to be be doing exactly the same thing, perhaps we can replace this with a call to createAddPullStream
and consolidate the progress accumulator logic there?
package.json
Outdated
@@ -74,7 +74,7 @@ | |||
"expose-loader": "^0.7.3", | |||
"form-data": "^2.3.1", | |||
"gulp": "^3.9.1", | |||
"interface-ipfs-core": "~0.31.19", | |||
"interface-ipfs-core": "^0.32.1", |
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.
@dryajov do not change ~ to ^, we use ~ until it gets to 1.0.0
this includes changes from #994