Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent implicit highWaterMark option set by through.obj #101

Closed
wants to merge 1 commit into from
Closed

Prevent implicit highWaterMark option set by through.obj #101

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Dec 9, 2015

through.obj(...) is documented as a shorthand for through({ objectMode: true }, ...) but in the source through2.js:88 it also sets the highWaterMark to 16.

This appears to cause problems when there's more than 16 files and there's nothing consuming the stream (i.e. gulp-notify is the last plugin in the pipeline and the stream is not returned) - see gulpjs/gulp#716. I think it also accounts for issues #96, #94 and #83 here.

Not sure if the change in this pull request is correct - I don't know much about node streams to be honest, but it seemed to correct the problem on my machine. On the other hand, the stream docs say highWaterMark has a default value of 16 anyway... so I don't know.

If there isn't an easy solution, maybe mention the need to consume the stream after notify() in the readme?

@coldasflux
Copy link

This is still an issue. Ran into the same.
Consuming the stream after notify with smth like pipe(size()) (gulp-size) will prevent this.

Would be nice to have things clarified, why the merge-request was not accepted.

@mikaelbr
Copy link
Owner

mikaelbr commented Nov 6, 2017

Hm. I see. I don't think this PR fixes it, though. As @parsnick mentioned, this is the default in node also. If I may ask, in what case don't you want to consume the producing stream? gulp-notify streams are intended as pass through streams, either before .dest or returned in a task.

@coldasflux
Copy link

coldasflux commented Nov 6, 2017

I'm using notify at the end of a pipe, like so...

gulp.src(...)
   .pipe(gulp.dest(...))
   .pipe(whatever())
   .pipe(notify({ onLast: true, message: '...'))

I want to be notified if the task has finished. If errors occur I won't be notified as I expect to be notified while watching.

@mikaelbr
Copy link
Owner

mikaelbr commented Nov 6, 2017

But you use that in a task, right? Should you return the stream created, and it'll work as expected, or am I missing something?

@coldasflux
Copy link

Returning the stream does it! I did miss this in certain tasks..

gulp.task('foo', function () {
   return gulp.src(...)
      .pipe(gulp.dest(...))
      .pipe(whatever())
      .pipe(notify({ onLast: true, message: '...'))
   ;
});

Thx a lot for pointing this out! :)

@mikaelbr mikaelbr closed this Apr 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants