send files HTTP request should stream #629
Conversation
Codecov Report
@@ Coverage Diff @@
## master #629 +/- ##
==========================================
- Coverage 83.86% 83.85% -0.02%
==========================================
Files 112 108 -4
Lines 1624 1604 -20
==========================================
- Hits 1362 1345 -17
+ Misses 262 259 -3
Continue to review full report at Codecov.
|
ghost
assigned 
pgte
changed the title
| // This does not work | ||
| // this.once('drain', () => { | ||
| // callback() | ||
| // }) |
There was a problem hiding this comment.
We do back-pressure per file item, but once inside a file we don't. So it's optimised for a lot of files, but not for large files. If that comes to be a problem, I think we should open a different issue to tackle it..
There was a problem hiding this comment.
Mind writing this note as the comment as well?
There was a problem hiding this comment.
Decided to fix individual file back-pressure in this PR: 782beb9
There was a problem hiding this comment.
Had to limit it to Node only though, because a browser HTTP request does not stream..
7403afc
src/files/create-add-stream.js
Outdated
| callback(null, ds) | ||
| }) | ||
| } | ||
| module.exports = (send) => SendFilesStream(send, 'add') |
There was a problem hiding this comment.
This file doesn't exist anymore, you need to rebase master onto this branch
src/files/add.js
Outdated
| qs.hash = opts.hash | ||
| } else if (opts.hashAlg != null) { | ||
| qs.hash = opts.hashAlg | ||
| } |
There was a problem hiding this comment.
Where did all of this options went?
There was a problem hiding this comment.
src/files/add.js
Outdated
|
|
||
| send.andTransform(request, (response, cb) => { | ||
| converter(ProgressStream.fromStream(opts.progress, response), cb) |
There was a problem hiding this comment.
Is this still doing Progress reports?
There was a problem hiding this comment.
| @@ -6,7 +6,6 @@ const isNode = require('detect-node') | |||
| const ndjson = require('ndjson') | |||
There was a problem hiding this comment.
While you are at it, wanna rename this file to send-request?
|
@diasdavid rebased. |
|
This is fantastic @pgte ! |
The HTTP multipart implementation provides a streaming interface but doesn't allow adding files mid-flight.
This PR does that and bases all the APIs that need to send files on that.
Should fix #628 (and, by association, ipfs/js-ipfs#823).