shortcut for stream output #30
Conversation
| @@ -68,6 +67,11 @@ module.exports = (files, options) => new Promise((resolve, reject) => { | |||
| if (files.length === 1) { | |||
| const fileObjKeys = Object.keys(files[0]) | |||
| if (fileObjKeys.length === 1 && fileObjKeys[0] === 'file') { | |||
| if (isStreamOutput()) { | |||
| const stream = fs.createReadStream(files[0].file); | |||
| resolve(output(stream)); | |||
There was a problem hiding this comment.
Sorry for super late response. I've completely missed this PR.
I believe it should be return resolve here or it will still read the files.
There was a problem hiding this comment.
No worries, we used my fork in the mean time ;)
You are absolutely right, missed the return there.
| @@ -112,21 +116,27 @@ module.exports = (files, options) => new Promise((resolve, reject) => { | |||
| } | |||
|
|
|||
| const childPromise = (isWindows && options.libPath !== 'pdftk') | |||
| ? execFile(options.libPath, args) | |||
| ? exec(`"${options.libPath}" ${args.join(' ')}`) | |||
There was a problem hiding this comment.
Are windows tests passing after this change? I know we had issues with this before in conjunction with files contaning spaces, ", , / etc.
There was a problem hiding this comment.
Unfortunately I can't run the tests locally. But I assume it works, since all filepaths are quoted on line 104.
For me and my team it does on windows when using it productively, but then again, we don't have spaces or special chars in the paths. In any case it does not work with execFile on windows for us.
| return stream; | ||
| } | ||
|
|
||
| return readFile(tmpFilePath).then(unlinkTempFile) |
There was a problem hiding this comment.
This breaks any non-stream output as the promise will resolve undefined which is later passed to the output function.
On closer inspection I believe this is a bug in the new unlinkTempFile:
const unlinkTempFile = (buffer) => {
new Promise((resolve) => {
fs.unlink(tmpFilePath, () => resolve(buffer));
})
};Ought to be
const unlinkTempFile = (buffer) =>
new Promise((resolve) =>
fs.unlink(tmpFilePath, () => resolve(buffer))
);There was a problem hiding this comment.
Sorry, completely forgot about this.. Yes, you are right. My teammate fixed it.
return promise
avoid reading the merged file(s) to a buffer if the requested output format is stream