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

Simplify the module to just use cb(null, file.clone()) #1

Merged
merged 2 commits into from
Apr 6, 2014
Merged

Conversation

ben-eb
Copy link
Contributor

@ben-eb ben-eb commented Apr 3, 2014

See this issue ben-eb/gulp-rsvg#2

Tested this out and it works pretty well. It lets you compose gulp tasks like so:

var gulp   = require('gulp');
    clone  = require('gulp-clone');
    concat = require('gulp-concat');
    es     = require('event-stream');
    uglify = require('gulp-uglify');

gulp.task('scripts', function() {
    var stream = gulp.src('*.js').pipe(concat('main.js'));

    var minify = stream
        .pipe(clone())
        .pipe(uglify())
        .pipe(gulp.dest('prod'));

    return es.merge(stream.pipe(gulp.dest('dev')), minify);
});

This would allow you to create two JS bundles; one concatenated and one concatenated and minified. 😃

@mariocasciaro
Copy link
Owner

I might be wrong (not tried it out), but your code seems creating 1 concatenated file in 'dev' and 2 uglified files in 'prod'
You should be able to do what you need to even without clone, in theory:

var stream = gulp.src('*.js')
  .pipe(concat('main.js'))
  .pipe(gulp.dest('dev'))
  .pipe(uglify())
  .pipe(gulp.dest('prod'));

Beside that, I'm not sure if changing the API has any advantage. I can't think of any situation where clone is not used in combination with a merge AND to bypass a certain pipeline of streams.
Comments on this are welcome.

@ben-eb
Copy link
Contributor Author

ben-eb commented Apr 4, 2014

That's not the case, you get two main.js files; one that has been run through the minifier (prod) and the other that hasn't (dev). With this change it would allow you to compose as many clone() calls together as you like. You could do something like this if you wanted:

var rename = require('gulp-rename');
// rest of modules...

gulp.task('scripts', function() {
    var stream = gulp.src('*.js');

    var concat = stream.pipe(clone()).pipe(concat('main.js'))

    var minify = concat
        .pipe(clone())
        .pipe(uglify())
        .pipe(rename('main.min.js'));

    return es.merge(stream, concat, minify).pipe(gulp.dest('out'));
});

e.g. Pipe all of the JS files to a directory, create a concatenated development version and create a production version in the same step.

So you have a lot more flexibility and it works how you would expect it to. I don't think it's really necessary to impose a method call to 'merge' the streams back together when you can use es.merge which takes care of it already. :-)

I guess you have a point with the multiple gulp.dest calls but using that logic wouldn't that invalidate this plugin? I.e don't bother cloning the stream at all, just write to disk instead!

@mariocasciaro
Copy link
Owner

Ok, I see it now. Thanks for explaining it again. The solution you are proposing is definitely less opinionated, so I would expose it as main functionality, but I'd still like to keep the old operating mode for convenience, I think it makes the code more readable for simple cases.
Could you make sure to implement an interface like this:

var clone = require('gulp-clone');
//The clone stream as you proposed
clone()

//old mode
var sink = clone.sink()
sink.tap()

@ben-eb
Copy link
Contributor Author

ben-eb commented Apr 5, 2014

OK, fair enough 😃

See the latest patch for the old behaviour.

mariocasciaro added a commit that referenced this pull request Apr 6, 2014
Simplify the module to just use cb(null, file.clone())
@mariocasciaro mariocasciaro merged commit cb15b47 into mariocasciaro:master Apr 6, 2014
@mariocasciaro
Copy link
Owner

Looks good, thanks for that. Will land in 1.0.0

@ben-eb ben-eb deleted the patch1 branch April 6, 2014 16:09
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