gulpjs / undertaker

Task registry that allows composition through series/parallel methods.
MIT License
200 stars 31 forks source link

Can't get task description and flags because returning wrapped function #65

Closed sttk closed 8 years ago

sttk commented 8 years ago

By modification that gulp.task(name) returns wrapped function instead of original function, gulp-cli can't get task description and flags now.

Since gulp.task(name).unwrap() returns original function, we can get description and flags by gulp.task(name).unwrap().description and gulp.task(name).unwrap().flags.

Is this intentional way?

phated commented 8 years ago

@sttk that was an unintentional regression with this change. This change needed to be implemented to fix https://github.com/gulpjs/gulp/issues/1492

Do you think we should mirror those properties onto the wrapper function? It feels like that leaks gulp details. I think the best solution would be to use unwrap() in gulp-cli. Thoughts?

sttk commented 8 years ago

@phated I've modified gulp-cli to adapt to both v1.0.0 and previous versions of undertaker in this PR.

I think the best solution would be to use unwrap() in gulp-cli.

I think it is OK, but does the name unwrap become official? Users could set task description and flags in following way:

gulp.task('clean', function() { ... });
gulp.task('clean').description = '...';

gulp.task('default', gulp.series(...));
gulp.task('default').description = '...';

And now users can set them as follows:

gulp.task('clean', function() { ... });
gulp.task('clean').unwrap().description = '...';

gulp.task('default', gulp.series(...));
gulp.task('default').unwrap().description = '...';

Or are these bad manners?

sttk commented 8 years ago

@phated By the way, if gulp.task(name) need not to return its original function, I think taskWrapper need not to be a function and is better to be an object. Because an object can have name property and would be suitable to have properties for a task such as description and flags

phated commented 8 years ago

@sttk I've been thinking about this all morning. The wrapper function should still be a function but the CLI should check for taskWrapper.description before attempting taskWrapper.unwrap().description (and similar for flags). That should support all the old ways of supplying flags/descriptions. Remember, flags and descriptions are something that gulp (specifically gulp-cli) care about and shouldn't leak into undertaker.

sttk commented 8 years ago

@phated description and flags are examples and what I want to say above are about any properties and about extensibility of undertaker or applications using it.

But this is a mere idea. I accept your saying that the wrapper should be a function.

sttk commented 8 years ago

@phated This issue was already solved by gulp-cli PR #87. Can I close this?

phated commented 8 years ago

@sttk thanks for reminding me. I forgot there was an issue about this opened over here.