gulpjs / gulp

A toolkit to automate & enhance your workflow
https://gulpjs.com
MIT License
33.01k stars 4.23k forks source link

[gulp 4] allow forward references to tasks #802

Closed alvint closed 9 years ago

alvint commented 9 years ago

Currently, forward references to tasks do not work in gulp 4 (and gulp 3, but I don't care about that):

gulp.task('default', gulp.series('clean', 'build', 'watch', 'server'));

gulp.task('clean', function () {
    // do something
});
...

This results in AssertionError: Task never defined: clean.

From a stylistic standpoint, many people prefer to place the broadest tasks first and then define component tasks later, since this is how most people think and design code. Also the current 'bottom up', Forth-like restriction is no longer necessary considering how gulp 4 works.

From a design standpoint, since tasks are interchangeable with functions now, and since functions (like most stuff in Javascript) can be referenced from anywhere within the current scope regardless of where it was declared, to be consistent tasks should work the same way.

alvint commented 9 years ago

Related to #355.

phated commented 9 years ago

I would suggest just declaring the function clean(){} below and then using the named function instead of by task string.

alvint commented 9 years ago

It would not be as obvious how to forward reference simple tasks (no function body) which themselves have component tasks. For example:

gulp.task('default', gulp.series('clean', 'build', 'watch', 'server'));

// How would you forward reference this task without unnecessarily rewriting it
// to include a function body?
gulp.task('clean', gulp.parallel('clean-this', 'clean-that'));

Additionally, it doesn't consider the consistency issue.

phated commented 9 years ago

We build the function at invocation time instead of runtime of the task because tasks would be extremely slow otherwise. That is just how JS works. All the use cases can be handled by named function expressions that call the gulp.series/parallel inside them.

heikki commented 9 years ago

If I forget possible implementation & performance problems this sort of makes sense. Consistency will produce less confused beginners.

Alternative? Further "encourage" plain function usage by removing task name support from gulp.series / gulp.parallel. :smile:

alvint commented 9 years ago

Sorry, I don't see how this would be any slower since you're using a registry anyway, and the increase in initial overhead would be negligible unless you had many, many thousands of tasks. Could you explain? Also what do you mean by, "everything in JS works that way"? I'm no javascript guru, but to my layman's eyes everything in JS works the opposite way (as mentioned, forward references are allowed). If it caused such degraded performance, you can bet it wouldn't be that way.

phated commented 9 years ago

@alvint the only thing that gets hoisted in the way you are suggesting are named function expressions. An anonymous function that is declared as var clean = function(){} will not be hoisted, while function clean(){} will be. Calling of functions, assignment of variables, etc are executed top-to-bottom. gulp.series/parallel behaving this way is the inherent way JS works because they are creating (essentially) anonymous functions because they can't be named.

phated commented 9 years ago

@heikki the new documentation is going to highly encourage using plain functions instead of strings. gulp.task will also allow taking a named function and use the name as your task name (my favorite thing in gulp4)

yocontra commented 9 years ago

I kind of agree with @alvint here - what's the point of a registry if everything needs to be defined in order? It seems super inflexible. If construction of the function is really a perf problem why can't we use caching to only construct once on first run?

Possible use case for this is splitting tasks across multiple files, which with the current impl would all have to be loaded in the right order.

alvint commented 9 years ago

@phated Sorry, but I don't see how that last comment is relevant to anything we're discussing here? Since we're talking about forward referencing tasks, and since you yourself suggested using functions to handle this, then clearly we're talking about named functions. Bringing up anonymous functions does nothing except cloud the issue.

I'm not one to mince words: if I offended you in some way, I apologize. I'm very willing to concede that you're quite knowledgeable (you are). You may or may not concede that I'm a knowledgeable person as you wish. But this conversation seems to be going in a manner that's not constructive to improving the product, which is the only thing I'm interested in. How do we end this and get back on track?

yocontra commented 9 years ago

@heikki I'm starting to think the same thing, maybe the string lookups in series/parallel aren't needed and will simplify things to remove them

phated commented 9 years ago

@alvint No offense, just commenting based on the stuff I've done so far. Looking into the registry, we could possibly add another function wrapper to support this but it seems like it might be a pain for task tree generation.

heikki commented 9 years ago

Readme sample gulpfile.js with plain functions:

var gulp = require('gulp');
var coffee = require('gulp-coffee');
var concat = require('gulp-concat');
var uglify = require('gulp-uglify');
var imagemin = require('gulp-imagemin');
var sourcemaps = require('gulp-sourcemaps');
var del = require('del');

var paths = {
  scripts: ['client/js/**/*.coffee', '!client/external/**/*.coffee'],
  images: 'client/img/**/*'
};

// Not all functions need to use streams
// A gulpfile is just another node program and you can use all packages available on npm
function clean(cb) {
  // You can use multiple globbing patterns as you would with `gulp.src`
  del(['build'], cb);
}

function scripts() {
  // Minify and copy all JavaScript (except vendor scripts)
  // with sourcemaps all the way down
  return gulp.src(paths.scripts)
    .pipe(sourcemaps.init())
      .pipe(coffee())
      .pipe(uglify())
      .pipe(concat('all.min.js'))
    .pipe(sourcemaps.write())
    .pipe(gulp.dest('build/js'));
}

// Copy all static images
function images() {
  return gulp.src(paths.images)
    // Pass in options to the task
    .pipe(imagemin({optimizationLevel: 5}))
    .pipe(gulp.dest('build/img'));
}

// Run a function when a file changes
function watch() {
  gulp.watch(paths.scripts, scripts);
  gulp.watch(paths.images, images);
}

var all = gulp.parallel(watch, scripts, images);

// The default task (called when you run `gulp` from cli)
gulp.task('default', gulp.series(clean, all));
alvint commented 9 years ago

@heikki Yep, it is doable with functions and that's how I do it myself in experimental projects where I'm using G4. But I don't think named tasks are going away any time soon; they're a victim of their own success. I imagine most people will not want to be forced to radically change their existing gulp.js files even to reap the new benefits. They're already working so why fix? And you can never overestimate the inertia of crusty old developers like me who are already used to doing things a certain way. And crusty old developers tend to have a lot of pull when it comes to deciding what technologies to use.

The reason I suggested this feature is that it's low-hanging fruit, and it's something crusty old developers can wrap their head around and immediately embrace, thus enticing them into G4. Then after a while the benefits of using functions will slowly trickle into their time- and code-addled brains and they'll embrace the newer, better way of doing things. But if you try to force people to use what (in their minds) is a radically different way of doing things just for some silly reason like "it's better", what you'll likely wind up with is a very popular G3 fork. That's not good for anyone because of the duplicated/wasted effort.

Also, there are practical considerations which may make upgrading existing gulp.js files to use functions more than a trivial effort in many cases. For example, I imagine many people will wind up with name conflicts since they'll wind up with both a plugin and a task named "sass". That's one clear advantage of named tasks: it's a separate namespace.

alvint commented 9 years ago

...and really, there should be no disadvantage to using named tasks because they should be 'tokenized' into function references by execution time anyway.

heikki commented 9 years ago

If we forget possible crusty-old-developer & implementation & performance problems, does it make sense? Brains tend to work better when imagining first advances and then drawbacks.

alvint commented 9 years ago

@heikki The continued support of named tasks or this particular enhancement? I think both make sense from the user side, but I'm not a G4 dev so they can speak to that aspect.

heikki commented 9 years ago

@alvint I meant gulpfile example i.e. allowing only functions to series / parallel and solving this issue as a side effect. After that any reference problems would be just normal javascript.

alvint commented 9 years ago

@heikki In that case you're right, but there would be no reason to keep named tasks at all (assuming gulp.run stayed away as well). My fear with that approach is that users will see in G4 a product that is too different from the product they're already used to and like, and be less inclined to adopt it. Ask Microsoft--people just don't like different when they already know how things work. And they really don't like different if it means they have to go back and change stuff that they don't want to think about anymore.

Like it or not, named tasks is a signature feature of Gulp and it's one the things people like about it. And it does add some modest utility like a separate namespace and the ability to use more characters in the name. And in most editors the differing syntax highlighting for strings makes the files more readable at a glance (ok, I'm reaching on that one).

heikki commented 9 years ago

Random thoughts, might be unclear :smile:

If series / parallel accepted only functions

Terminology:

Advances:

Drawbacks:

Related issues:

alvint commented 9 years ago

Minor edit suggestion: from what I understand of the current G4 proposal anonymous functions are private tasks and named functions are public tasks (available from the command line).

That actually brings up another argument for having named tasks: the suggested G4 system as it stands would only allow forward references to tasks if the tasks were also made public. That completely subverts the motivation for having forward references in the first place (to keep the 'important' tasks at the top of the file and the 'component' tasks at the bottom). It's the important tasks that you want to make public and you want to keep the component tasks private.

phated commented 9 years ago

That is incorrect. Anonymous functions aren't private tasks. Functions that are not registered by gulp.task are private.

alvint commented 9 years ago

My mistake (I think that was suggested at some point).

alvint commented 9 years ago

...although if there are no named tasks, you could argue that you're running out of reasons to keep gulp.task around as well. Is the only reason it would still be needed to make tasks public?

phated commented 9 years ago

@alvint correct. gulp.task is only used to expose things to the CLI.

alvint commented 9 years ago

I would guess that would cause a lot of confusion since its function would no longer be to create tasks (no point having two different ways to create tasks) but to accept a list of tasks that should be made public. How about an API like gulp.export(func1, func2, ...) to avoid it?

alvint commented 9 years ago

BTW what's the name of the default task, or does a task always need to be named when you run gulp?

alvint commented 9 years ago

Just tossing stuff out, but with a gulp.export API you could specify that the first task listed is the default task.

yocontra commented 9 years ago

@alvint To reply to your earlier comment about not breaking too much, there is a reason why this is gulp 4 and not gulp 3.9. We are radically breaking everything to improve the product. If people don't want to change they can keep using 3.x stuff

alvint commented 9 years ago

@contra Heck, I like the new function-based stuff and I'm using it myself, but I can certainly see why people may want to stick with named tasks. I'm a relative newcomer to Gulp so I don't have an especially large body of work based on it to maintain. And remember that the popularity and acclaim of Gulp comes from the people who use it. The elevated status and visibility of this project is thanks to them (the internet is full of well written code that nobody knows about). People might be put off by an attitude that (to them) seems to be, "Thanks for making us what we are. Now f**k you."

It would take little effort to keep named tasks in, and if done correctly they'll be replaced with their respective functions by the time execution rolls around* anyway, so there's zero performance impact at execution and negligible overhead during configuration. Why not keep it in to appease the masses?

* I'm defining "execution time" as the time the first task is executed, not the time the gulp.js file is run (which is configuration time).

yocontra commented 9 years ago

@alvint Your whole argument is based on the premise that we're saying "fuck you" to people by making breaking changes. This is not based in reality - projects make breaking changes all the time. This is part of the natural progression of life.

I understand that we could probably keep supporting string names really easily, but in gulp 4 we are trying to rethink everything based on what we have learned in the last year to remove features that aren't needed and add ones that are missing (better error management, sourcemaps, incremental builds, etc.). gulp 4 will absolutely have breaking changes, and this is a good thing. If a project goes 1 year without any breaking changes then it's either finished or the maintainer is asleep at the wheel.

For this specific feature, we are trying to reduce confusion for users by optimizing the # of ways they can peel this apple (defining a task). Would you rather be given 10 peeling knives and you have to decide which one to use or be handed the best peeling knife?

If you don't want to migrate then you can stay at your current version. Bug fixes and new features will still come to you via submodule updates which is how this entire project is organized.

alvint commented 9 years ago

@contra That's not quite what I'm saying. Sometimes breaking changes do have to be made if they're necessary to move forward. The issue here is wether the breaking changes being considered here are necessary, or wether it was simply decided that using functions is the "cooler"/more elegant way to handle tasks and therefore everyone should be made to use them. Yes, it would take a little extra coding effort to include named tasks (not that much), but I'm not aware of any technical limitation which forces them to go away. Please correct me if I'm wrong.

I know you never asked anyone to depend on your code and in the end it's your project and you can do with it what you want. I'm just trying to play "devil's advocate" to let you know how how such actions may be perceived from without.

alvint commented 9 years ago

Don't get me wrong--I think you all have done a great job, and the fact that I'm spending so much of my (fairly valuable) time on this thread is proof of that. I'm just normally more critical of the things I have high expectations in to try to help push them forward. I'm aware of the self serving psychology behind that--everyone wants to claim they had a hand in shaping a winner--but I also legitimately think it would be best for the short and long term health of the project to keep named tasks.

But we're way OT for this issue. The decision on wether to keep named tasks is yours. This issue is simply to request forward references if you do keep them.

yocontra commented 9 years ago

Sometimes breaking changes do have to be made if they're necessary to move forward

@alvint That's the conversation I was trying to have until you came in and disrupted it. I simply asked "let's think about this - do we really need string lookups?" and you start obstructing. At no point did I issue a verdict and say we were doing anything. If we could just return to my initial question: Do we need string lookups?

alvint commented 9 years ago

@contra I like to think that I moved it forward. :P But enough of me flapping my gums. BTW, how do you declare which task is the default task in 4?

yocontra commented 9 years ago

@alvint Declaring a default task with either a string or a named function

alvint commented 9 years ago

@contra I guess I'm getting my languages confused. Can you have a function named "default" in js?

alvint commented 9 years ago

@contra To clarify my question, @heikki mentioned that gulp.task will also allow taking a named function and use the function name as your task name. I assumed that meant a signature like

gulp.task(function foo() {
    ...
});

...to declare a task named "foo". So my question is how to declare the default task using this form, since "default" is a keyword. If named tasks do go away there would be no gulp.task('task-name', function () { form anymore.

alvint commented 9 years ago

damned buttons...

yocontra commented 9 years ago

@alvint Good catch, you wouldn't be able to do that.

alvint commented 9 years ago

@contra Shameless plug of the gulp.exports suggestion above. :)

heikki commented 9 years ago

To clarify my question, @heikki mentioned that gulp.task will also allow taking a named function and use the function name as your task name.

@phated did.

If named tasks do go away there would be no gulp.task('task-name', function () { form anymore.

I don't follow now.. why do you think that form goes away? AFAIK this modified 4.0 sample gulpfile is what has been talked so far.

alvint commented 9 years ago

@heikki The issue is if named tasks were removed, the function signature gulp.task(String, functionName() {...}) would then only be needed for this one use case (setting the default task). Specifically, only the literal form gulp.task('default', myDefaultTask() {...}) would ever really be needed, since the form gulp.task(myTask() {...}) would take care of all other cases where you need to declare a public task. The String parameter would have no use in any other case (except possibly exporting the task by a different name, but there are much better ways to do that). In the case of the default task, it would be better to just use something like gulp.defaultTask(myDefaultTask() {...}) instead of a String parameter that's always 'default'.

Furthermore, even the form gulp.task(functionName() {...}) is not very elegant, since (a) now you have multiple ways of creating a task instead of just one, (b) you can only declare one task as public at a time, (c) you can't just look at one line to see a list of all public tasks--instead you have to hunt through the code, and (d) it's wordier since you have to repeat gulp.task for every task you want to make public. It's better and more intuitive to just create all tasks with standard function declarations, then declare all public tasks at once with something like gulp.exports(myTask1, myTask2, ...).

The exports function would take a list of task functions (or also allow objects if you need more control). By convention the first task function listed could be the default task. The object could look something like:

{
    "name": "publicTaskName",
    "task": myTask1,
    "someOtherNeededArgument", "foo"
}

This approach is much more Javascript-like, is more readable, is less wordy, and hides configuration options unless you really need them. Another advantage is that by using an object parameter you could export anonymous functions as well as named functions, if that's how you roll.

heikki commented 9 years ago

TL;DR

If named tasks do go away there would be no gulp.task('task-name', function () { form anymore.

Are you suggesting that named tasks should be removed or do you think someone is about to remove them? Your writing is pretty confusing from time to time.

alvint commented 9 years ago

@heikki As I said, I now use the function version of tasks myself, and I agree they provide real advantages in simplicity. And if you're gonna do it, then DO IT. Make an API so sparkly clean that you'd let your first-born eat off of it.

yocontra commented 9 years ago

@alvint Nobody was proposing that we keep string names just for default. You keep making assumptions and putting words in my mouth. Obviously if we removed string lookups there would be a new way to define tasks for the CLI

heikki commented 9 years ago

I seem to be using string version for task definitions atm because it makes them stand out from the rest of the stuff. Some task names would be awkward impossible to achieve without it: build:dist?

IMO string lookups should be removed from series/parallel making them deal with functions only.

alvint commented 9 years ago

@contra Umm...I never said any such thing?

geekflyer commented 9 years ago

@alvint

geekflyer commented 9 years ago

Back to the original topic: Regardless of whether named functions or string tasks - something like forward references could be very useful for splitting up your gulpfile into multiple files, as there the named function hoisting doesn't work. In my current project we have a relatively large gulp codebase with lots of custom code (split across 1 central gulpfile and 8 subfiles) and loading those subfiles in the right order took some thoughts.

Nevertheless I have to say I can live with the current behaviour and it's not super important for me. Doing it properly the subfiles just contain loosely coupled tasks and the series / parallel orchestration happens in the single central gulpfile. So we simply load all the subfiles in the beginning of the central gulpfile and then orchestrate the stuff into 'higher level' tasks afterwards.