hughsk / vinyl-map

Map vinyl files' contents as strings
MIT License
36 stars 7 forks source link

Emit an error when file.contents is a stream, closes #3 #4

Closed lazd closed 10 years ago

lazd commented 10 years ago

I'm not sure about the error message (vinyl-map: This plugin does not support streams), but this does the trick.

hughsk commented 10 years ago

Hey @lazd, thanks for taking the time to make a pull request!

vinyl-map isn't a gulp plugin, hence the lack of a gulp-* prefix. It still adheres to most of the gulp conventions by converting file.contents streams -> buffers -> streams, with that note on shoehorning streaming functionality being a relatively new one that I feel might be a little too vague.

If we were to remove this functionality, there's not any benefit over using map-stream or through2 instead. Maybe this is a better choice for gulp-handlebars if you were looking to stick to gulp's plugin guidelines?

lazd commented 10 years ago

I suppose the benefit of removing this functionality is that vinyl-map can double as a simple tool to aid in the development of gulp plugins, handling the boilerplate checking of file type and mapping calls to a method. If that's not the intended purpose, something else can surely fill the need.

If users want to use vinyl-map with streams outside of a gulp plugin but still in the context of gulp, they can add in gulp-buffer to make sure everything comes in as a buffer before hand.

As far as the note on shoehorning, I talked to @contrahax today on IRC and he noted that, with the stream -> buffer -> stream conversion in place, large files ran through multiple plugins can end up allocating many times their size in memory, hence the addition to the guidelines.

What do you think?

hughsk commented 10 years ago

Yeah, that's a valid concern for plugins if they're being put together in a long pipeline on a large project. However, you're still dealing with reallocating memory when using buffers, because they have to be converted to strings and then back to buffers again for each processing step. To best conserve memory vinyl-map would be better off used with the sync processing steps in a single handler:

gulp.src('./*.coffee')
  .pipe(require('gulp-concat')())
  .pipe(require('vinyl-map')(function(contents, file) {
    contents = contents.toString()
    contents = coffee(contents)
    contents = uglify(contents)
    // ..etc
    return new Buffer(contents)
  })
  .pipe(require('gulp-size')())
  .pipe(gulp.dest('./'))

Which should result in less memory allocation overall than using the individual plugins, assuming I switch vinyl-map over to use bl.

I actually wrote vinyl-map so that people didn't need to write as many gulp plugins, and instead just use modules directly, in the hopes of making gulp more npm-friendly – I don't mind if you want to fork this module or write an alternative which meets your needs :)