sindresorhus / gulp-template

Render/precompile Lodash templates
MIT License
289 stars 77 forks source link

Must handle streams and null file.contents #2

Closed phated closed 10 years ago

phated commented 10 years ago

Currently, you are always replacing the file.contents with a Buffer. You need to handle a streaming or null file.contents. Handling streams is outlined here and I handle each type in gulp-jade for an example.

sindresorhus commented 10 years ago

You should probably dogfood that ;) I looked at: https://github.com/wearefractal/gulp-coffee/blob/master/index.js

I don't like how every plugin has to implement that streaming check boilerplate. Seems like it could be handled by gulp by setting a flag or something.

// @Contra

sindresorhus commented 10 years ago

Also, docs says:

Your plugin doesn't have to handle this case

And when would file.contents be null?

phated commented 10 years ago

I am not fractal, and all of my packages dogfood the concept. Also, note Rule 7 from the guidelines

file.contents should always go out the same way it came in Respect buffered, streaming, and non-read files as well as folders

file.contents is null when a consumer sets the read option to false - https://github.com/wearefractal/gulp#optionsread

sindresorhus commented 10 years ago

file.contents is null when a consumer sets the read option to false - https://github.com/wearefractal/gulp#optionsread

I don't understand that option. What's the point of it? If nothing is being read, nothing is being written. You could just as easily just comment out the task.

phated commented 10 years ago

https://github.com/wearefractal/gulp/issues/14

sindresorhus commented 10 years ago

Thanks, though I still think gulp could handle that for us. Kinda slippery slope to make every task having to care about every added option.

yocontra commented 10 years ago

What do you think would be the best way for gulp to handle this for plugins? I can put more in gulp-util for handling the stream->buffer->stream case but for the null files thats just one line to ignore it and pass it along.

phated commented 10 years ago

I don't think the pattern is that complicated, since you are only passing the file object along once and just use isBuffer or isStream utils to handle those cases (if you want to be a good citizen). I do think the stream->buffer->stream case should get a util though.

sindresorhus commented 10 years ago

What do you think would be the best way for gulp to handle this for plugins?

I'm not sure. I just know from creating grunt plugins that I really don't like writing moot boilerplate. Maybe I could add a prop on module.exports to signal if I support streams, buffers or both. Though I don't really like that either. Maybe a utility on gutil would be the best thing.

null files thats just one line to ignore it and pass it along.

Not the size, more the the fact that every single plugin has to care.

I don't think the pattern is that complicated

It's not complicated at all, it's just boring and unDRY.

yocontra commented 10 years ago

@sindresorhus A plugin wrapper on util for plugins that only care about Buffers (probably 99% of all plugins) seems like the best way to me. Adding flags to module.exports takes away the best feature of gulp (IMO) which is that all plugins are just vanilla streams.

Related https://github.com/wearefractal/gulp-util/issues/11

sindresorhus commented 10 years ago

Works for me :)

sindresorhus commented 10 years ago

fixed

joaomilho commented 9 years ago

Just stumbled on this today, and the reference that supposedly fixes it on gulp util says "Changed my mind, not doing this". So what is the solution for a stream?

kevva commented 9 years ago

@joaomilho, this.

joaomilho commented 9 years ago

@kevva , this is actually the problem. My solution was to use vinyl-source-buffer instead of vinyl-source-stream.

kevva commented 9 years ago

This issue wasn't about stream support, but fixing cases where streams where used but not handled.