Add support for streaming output with option

[gpoole]

This change adds support for streaming output through a new buffer option to disable using buffers, which works around buffer size limitations I ran into when the output zip file is larger than Buffer MAX_LENGTH (1-2GB currently). This change also adds the following user-friendly error message when the buffer size is exceeded during zipping:

The output zip file is too big to store in a buffer (larger than Buffer MAX_LENGTH). To output a stream instead, set the gulp-zip buffer option to 'false'.

I've added test cases for the message and to make the vinyl file contents are a stream.

I haven't added a test to see if the stream can be successfully read since decompress-unzip unfortunately doesn't support streams. Since the stream used is just the original unbuffered output of yazl I thought the other tests might already cover it?

---

IssueHunt Summary

Referenced issues

This pull request has been submitted to:

• #86: stream option is not working

---

[sindresorhus]

Why make it an option? Could you not check file.isStream() and just do it if true?

[gpoole]

I also originally wondered that but to explain my thinking here:

1. We could have buffers and streams coming in, so we'd only be basing that decision off the first one we see
2. To me, wanting to stream files in doesn't imply wanting the result as a stream as well, mainly since streams are not as well supported in the gulp ecosystem and may cause problems down the line
3. I think having this as a default-off option would be a minor change, whereas taking the setting from the input files could be breaking since it would change the output from a buffer to a stream wherever streams are coming in

I'm happy to change it if you think I'm missing the mark there though.

[sindresorhus]

1. I don't think that's possible. A Gulp pipeline is either fully streamed or not at all, since even if only a single plugin in a pipeline doesn't support streaming, it will have to buffer for that plugin and it's all moot.
2. I'm ok with doing a major release for this.

I would prefer no option for this. We can mention streaming support and how to use it gulp.src(..., {buffer: false}).

[gpoole]

I think it's uncommon but definitely possible to have multiple modes on the Vinyl stream aside from plugin support. This example is a bit contrived, but I could do:

const mergeStream = require('merge-stream');
const gulp = require('gulp');
const gulpZip = require('gulp-zip');

exports.default = function() {
  return mergeStream(
    gulp.src('*.mp4', { buffer: false }),
    gulp.src('*.txt'),
  )
  .pipe(gulpZip())
  .pipe(gulp.dest('out/myzip.zip'));
}

I think I'd have a race condition where I'll either get a streamed or a buffered zip depending on which file arrives first, which I don't think should happen, and that also means I can't force streaming on if I'm having the oversized buffer problem.

What about instead if I changed it so that an oversized buffer error automatically switches the output to streaming and I'll add a note about it to the readme? Then we could avoid the option and automatically handle the error.

[sindresorhus]

We cannot automatically switch to streaming as it affects the plugins coming after this one.

I think you should rather bring this up on the Gulp/Vinyl issue tracker. Plugins should not be opinionated about this. They should support either buffer or both and adhere to the mode Gulp tells them.

[gpoole]

Sorry for the long delay on responding here. Yeah I agree that auto switching is the wrong way to go and would be confusing.

The problem I have with doing it automatically is that the file contents being buffered or streams is a property of each Vinyl file, but not of the stream as a whole. There's no API to check the streaming/buffered mode of the whole stream and it's possible to construct valid streams with mixed content, so my interpretation is that this is an intentional and quite useful feature of Gulp. I believe that plugins need to respect the mode of each file in the stream and it's not correct to infer the nature of every file in the stream from one instance. If I change this to automatically switch mode based on the first file it will introduce inconsistent behavior for mixed content streams like the one I mentioned above (the output could alternate between streaming and buffered depending on which file comes through the pipe first).

I think that given that, my opinion is that an option is really the only consistent and safe way to handle this. It's unambiguous and discoverable and gives the users of the plugin the choice without introducing the potential for inconsistent behaviour.

[sindresorhus]

Seems like an option is the only choice, yes, but I also think your use-case is very obscure, and that it would be easier for you to just make everything streaming. 🤷🏻‍♂️

[gpoole]

Yeah it isn't popular to do that, but I think that's a shame because in my opinion it's one of the coolest Gulp/Vinyl features that the API is based on regular streams and for example merging and splitting can be done at the stream level without Gulp even needing to know or provide APIs for it. I think it's a very powerful feature! That's why I think it's worth us supporting it with this change.

So if I put the option back in would you be ok with that?

[sindresorhus]

So if I put the option back in would you be ok with that?

Yes, but it should be clearly documented in what case the option is useful.

[gpoole]

Ok the option is back but renamed to buffer to match the gulp.src buffer option which I hope should make it easy to interpret along with the docs.

[gpoole]

I'll investigate the failing Node 14 test when I've got more time later this week.

[sindresorhus]

Yes, but it should be clearly documented in what case the option is useful.

[gpoole]

Hopefully clarified that now.

[sindresorhus]

It's not really clearer. As a user, I would have liked to know why the option exists when there's the gulp.src's buffer option.

[gpoole]

I felt that was explained in the paragraph above. Do you want me to remove the comparison or add more detail here?

[sindresorhus]

Add more detail.

[sindresorhus]

This is just repeating what's on line 60. I think the option docs should explain why it exists when there's already such option for gulp.src.

[gpoole]

Would something like this work:

We use this option instead of relying on gulp.src's buffer option because we are mapping many input files to one output file and can't reliably detect what the output mode should be based on the inputs, since Vinyl streams could contain mixed streaming and buffered content.

[sindresorhus]

Yes, that's good.

[sindresorhus]

Bump