add clarification to the README

[yocontra]

google/material-design-lite#790

People are using this in weird ways, can you add a clarification to the README that this module should only be used if:

• you need to run one command per file in the stream
• you need templating per file in the stream

and to use child_process otherwise?

Empty globs will start throwing errors in 4.0 as well. Thanks!

[sun-zheng-an]

Yes, I know it's weird to pass '' to make the command run exactly one time.

But many people are doing so because the child_process.exec is not so handy (you need to handle the output by your own) to use, especially for the people not familiar with Node.js (they don't know the existence of child_process and just use gulp as a build tool). This is the main reason why I create this plugin. It can save lines of code when someone needs to run some shell commands.

[yocontra]

@sun-zheng-an Why not make another module that simply runs a command that wraps child_process.exec? run('command', cb)? I'm sure that already exists. We are erroring on empty src streams in gulp 4.0 so what you are recommending is broken, and we actively tell people not to follow this anti-pattern.

[sun-zheng-an]

Why not make another module that simply runs a command that wraps child_process.exec?

Because it's weird to include two different module for running shell command in gulp.

We are erroring on empty src streams in gulp 4.0 so what you are recommending is broken, and we actively tell people not to follow this anti-pattern.

I tell people use this anti-pattern just for convenience (I know it's weird, but it works for a long time).

Can I tell people to use gulp.src('.') instead if the gulp.src('') will explode in 4.0? I hope they can update to 4.0 smoothly instead of changing much code.

[yocontra]

@sun-zheng-an It isn't weird at all to include two different modules that do two different things. One is for running commands as a stream, one is just for running commands. Making this module do both things is crazy and you are causing a bunch of people to do really stupid things by telling them to do something they absolutely shouldn't be doing.

We will do everything possible in gulp 4.0 to stop people from using plugins in this way, including erroring on dummy .src() calls.

[sun-zheng-an]

@contra

We actively tell people not to follow this anti-pattern.

Could you give me docs or discussions for explaining why this is considered harmful? I will put it into the clarification.

[sun-zheng-an]

Yeah, gulp-shell is promoting huge anti-patterns. I've opened an issue about this and it was closed with no resolution.

From gulpjs/gulp#1353

@contra I have never closed this issue at all and keep waiting for your reply.

[JohnAlbin]

Based on this comment gulpjs/gulp#1353 (comment)

It looks like the following code is an alternative to gulp-shell that is not an anti-pattern.

var spawn = require('child_process').spawn;

gulp.task('example', ['dependency'], function(cb) {
  var flags = [
    '--path',
    'some/path',
    '--verbose'
  ];
  var cmd = spawn('some-command', flags, {stdio: 'inherit'});
  return cmd.on('close', cb);
});

[yocontra]

@JohnAlbin Sure, if you want to write it in the most verbose way possible. You'd be better off using any of the tens of modules that make that sample one line of code (ie https://github.com/danielhusar/exec-chainable):

gulp.task('example', ['dependency'], function(){
  return exec('some-command --path some/path --verbose');
});

or in coffee

gulp.task 'example', -> exec 'whatever-command --arg=1'

I made this same point a few comments ago: there is no reason for running a command to be a gulp plugin if you are not running the command based on files in the stream. It's faster, simpler, cleaner, and more maintainable to just run a command using standard practices than to do some crazy hack with streams.

[JohnAlbin]

@contra I was just providing an example for others to follow since no one else had done so and it took me a while to figure it out. Thank you for another example, though I won't use a module that is on v0.0.3. I prefer Node.js authors that will commit to supporting a semver-friendly 1.0 API in their modules.

I'll stick with core Node.js APIs for this particular problem. Here's a good comparison of spawn vs. exec that (while old) is really useful if others are unfamiliar with them (as I was). http://www.hacksparrow.com/difference-between-spawn-and-exec-of-node-js-child_process.html

BTW, even though I just stopped using this gulp plugin, I still don't know why the things it does are an anti-pattern. Neither does @sun-zheng-an (see #55 (comment) above.) Is it just that there are existing npm modules that can do this without being gulp-specific? or is there some other anti-pattern you were referring to?

[yocontra]

@JohnAlbin Okay, it was a random module out of hundreds that popped up when searching "exec" on npm. We don't need to discuss opinions about semantic versioning.

I've outlined why it is an anti-pattern like ten times in this issue, I don't understand how I can be any clearer:

gulp.src('').pipe(shell('run some command')) to run a single command makes as much sense as setting i = 123 with var i = ((function(){}).i = [])[NaN] = 123;

I opened this issue as a courtesy to let everyone know that this anti-pattern has been invalidated in an upcoming release, and the code you are recommending will stop working. I'm going to close this now because I don't think there is anywhere to go from here. The change has been made, everyone has been informed.

[strpipe]

Well doing something like 'child-process'.spawn is kinda upside down.
That is definetly no good design.

I have yet to come across a real Definition of an anti-pattern that also explains why Dependency INjection is no anti-pattern. Biggest security-leak imo.