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

Provide function for building a pipeline #1433

Closed
gagern opened this issue Dec 7, 2015 · 6 comments
Closed

Provide function for building a pipeline #1433

gagern opened this issue Dec 7, 2015 · 6 comments

Comments

@gagern
Copy link

gagern commented Dec 7, 2015

Stream pipelines are a nice elementary building block. However, errors don't usually propagate down the stream. I wonder whether it would make sense to have gulp export a function like this:

gulp.pipeline = function(first) {
    var stream = first;
    for (var i = 1; i < arguments.length; ++i) {
        var next = arguments[i];
        stream.on("error", next.emit.bind(next, "error"));
        stream = stream.pipe(next);
    }
    return stream;
}

Then you could change the documentation to suggest that users use this function instead of manually building up the pipeline. One example from the docs would now read

gulp.task('scripts', ['clean'], function() {
  // Minify and copy all JavaScript (except vendor scripts)
  // with sourcemaps all the way down
  return gulp.pipeline(
    gulp.src(paths.scripts),
    sourcemaps.init(),
      coffee(),
      uglify(),
      concat('all.min.js'),
    sourcemaps.write(),
    gulp.dest('build/js'));
});

I think that dropping the calls to .pipe adds clarity to this formulation. And the fact that gulp has ultimate control over how the pipeline gets built means that it can be more flexible in how to handle these things, e.g. in terms of error propagation. In doing so it might take attributes of the individual streams into account, which might avoid some of the monkey-patching we see today.

Has this been discussed before?
How do you feel about this idea? Should I write a pull request for this?
Are there any specific issues affected by this which should be investigated further?

@phated
Copy link
Member

phated commented Dec 7, 2015

Somewhat related to #1269 - we don't bring things into core that already exist in userland and don't fit our very narrow scope. For the 4.0 release, I'd love to have some docs contributed that list common and recommended userland modules, like https://www.npmjs.com/package/mississippi.

If you'd like to contribute the beginnings of that documentation, that would be awesome.

@phated phated closed this as completed Dec 7, 2015
@gagern
Copy link
Author

gagern commented Dec 24, 2015

Indeed, mississippi or its components pump or pumpify seem to solve the same problem my code snippet addresses, albeit with apparently larger overhead due to additional functionality not needed here.

When adjusting the docs, I'd personally suggest using this functionality everywhere .pipe is being used so far. Which means most of the examples would require some third party module. Do you really think this kind of omnipresent use doesn't warrant inclusion into the core module? Or do you want to suggest using such modules only in some scenarios? If so, how would you distinguish them? I fear that suggesting different approaches only adds to the confusion.

Comparing miss.pipe to miss.pipeline, which would you prefer? I think miss.pipe with its callback function seems more appropriate. Changing all examples to that might turn the fact that a gukp task may also return a stream into a more obscure feature, but on the other hand, I don't see any harm in that since it's not that needed any more either if people start using miss.pipe or pump.

@yocontra
Copy link
Member

@gagern We're pretty dedicated on sticking to the normal stream usage - the idea is that if you know node, you know gulp. We don't like to invent any of our own stuff, which is why we stick with core streams and simple async functions for everything.

@gagern
Copy link
Author

gagern commented Dec 24, 2015

@contra I know and appreciate that idea. One thing I like about gulp is that I can understand its mechanics pretty easily. But the fact remains that the .pipe method of core node streams does not perform sufficient error propagation. So you can either let errors bubble to the default handler and get reported out of context (which is the default in current usage), or you have to provide some mechanism to propagate errors from intermediate pipeline stages to somewhere where gulp can intercept them and present them in context. You still have default streams that way, but not the .pipe method but something which does some .on('error', …) handling as well, without burdening the user with this.

@yocontra
Copy link
Member

@gagern We have another issue open about how we should address this - would love for you to chime in there #358

@gagern
Copy link
Author

gagern commented Dec 26, 2015

@contra, while #358 is certainly interesting to read, and I did add my $0.02 to it, it's focus is the unpipe event associated with an error. Here I'm not worried about unpiping, but about error events ending up at the default stream error handler instead of some place where gulp will know that the task in question failed. These are in my opinion distinct problems, and while some solutions address both of them, it is far from obvious to me that they should be addressed together. Anyway, in #358 (comment) I described how my suggestion here (of providing a gulp.pipeline function, which may well be backed by pump, pumpify, missisippi or whatever behind the scenes) could help with that problem, too.

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

No branches or pull requests

3 participants