Set the FileStreamConverter explicitly #701
Conversation
ghost
assigned 
|
@pgte Can you especially review:
|
src/files/add-pull-stream.js
Outdated
| module.exports = (send) => { | ||
| return (options) => { | ||
| options = options || {} | ||
| options.converter = FileStreamConverter |
There was a problem hiding this comment.
If add-readable-stream already sets the converter, you don't need to set it here.
There was a problem hiding this comment.
I don't need to, but I could :) In the current way it would make sure that the converter for the PullStream is always the FileStreamConverter no matter which value it had. I think that makes sense, doesn't it?
src/files/add-pull-stream.js
Outdated
| @@ -1,6 +1,13 @@ | |||
| 'use strict' | |||
|
|
|||
| const SendFilesStream = require('../utils/send-files-stream') | |||
| const FileStreamConverter = require('../utils/file-stream-converter') | |||
There was a problem hiding this comment.
I think file-stream-converter is still to ambiguous. I would call it file-result-stream-converter.
| convertedResponse.on('data', (d) => retStream.push(d)) | ||
| response.pipe(convertedResponse) | ||
| if (options.converter) { | ||
| response.on('data', (d) => { |
There was a problem hiding this comment.
Because you're not returning the converted response, the back-pressure here doesn't work.
Instead, I suggest that you create a duplex stream from the input stream and the converted response, and that's what you would return. (This way, the client can do back-pressure: if the result data is being processed too slowly and the server implements back-pressure properly, the back-pressure should propagate all the way to the server and back into the request stream).
There was a problem hiding this comment.
Perhaps you could use duplexify to create a duplex stream from the writable (the client pushing files) and the readable (the result stream).
There was a problem hiding this comment.
Is this a bug in the original code? I don't see how my code is different from the original one in case a converter is set.
There was a problem hiding this comment.
@vmx I see, this is a problem that was already there before, I only spotted it now.
Feel free to ignore this (and we should probably open an issue with this one). :)
There was a problem hiding this comment.
Did the issue get created?
The `SendFilesStream` used to run the `FileResultStreamConverter` automatically. With making it an option to use a converter, `SendFilesStream` can be used outside of the Files API, e.g. for the DAG API. Closes #696.
vmx
force-pushed
the
explicit-converter
branch
from
6d90b46 to
2c47b4f
Compare
|
@pgte I did a force push with the naming suggestion. I should have addressed everything now. Could you please open an issue for the back-pressure problem? I can get what the problem is, so you'd be better explaining it. |
|
The tests passed on Node 6 (https://travis-ci.org/ipfs/js-ipfs-api/jobs/346307987#L3540) and 8 (https://travis-ci.org/ipfs/js-ipfs-api/jobs/346307988#L5306) on Travis. @diasdavid Is that good enough for a merge? |
|
@vmx I liked your proposal to disable browser tests in Travis and Circle meanwhile (just pass on Jenkins). Wanna do that first? |
The
SendFilesStreamused to run theFileStreamConverterautomatically.With making it an option to use a converter,
SendFilesStreamcan beused outside of the Files API, e.g. for the DAG API.