Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[gulp 4] allow forward references to tasks #802

Closed
alvint opened this issue Dec 1, 2014 · 77 comments
Closed

[gulp 4] allow forward references to tasks #802

alvint opened this issue Dec 1, 2014 · 77 comments

Comments

@alvint
Copy link

alvint commented Dec 1, 2014

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
Copy link
Author

alvint commented Dec 2, 2014

Related to #355.

@phated
Copy link
Member

phated commented Dec 2, 2014

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

@alvint
Copy link
Author

alvint commented Dec 2, 2014

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
Copy link
Member

phated commented Dec 2, 2014

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
Copy link
Contributor

heikki commented Dec 2, 2014

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. 😄

@alvint
Copy link
Author

alvint commented Dec 2, 2014

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
Copy link
Member

phated commented Dec 2, 2014

@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
Copy link
Member

phated commented Dec 2, 2014

@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
Copy link
Member

yocontra commented Dec 2, 2014

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
Copy link
Author

alvint commented Dec 2, 2014

@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
Copy link
Member

yocontra commented Dec 2, 2014

@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
Copy link
Member

phated commented Dec 2, 2014

@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
Copy link
Contributor

heikki commented Dec 2, 2014

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
Copy link
Author

alvint commented Dec 2, 2014

@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
Copy link
Author

alvint commented Dec 2, 2014

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

@heikki
Copy link
Contributor

heikki commented Dec 2, 2014

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
Copy link
Author

alvint commented Dec 2, 2014

@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
Copy link
Contributor

heikki commented Dec 2, 2014

@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
Copy link
Author

alvint commented Dec 2, 2014

@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
Copy link
Contributor

heikki commented Dec 2, 2014

Random thoughts, might be unclear 😄

If series / parallel accepted only functions

Terminology:

  • task – what you run from cli
  • src – read files
  • dest – write files
  • watch – react to file changes
  • series / parallel – compose functions

Advances:

  • just necessary tasks – easier to use
  • functions aka private tasks
  • teaches javascript
  • same problems for all
  • simpler implementation
  • clear method signatures
  • clear terminology

Drawbacks:

  • crusty old developers aka old habits
  • too much changes
  • enforcing things

Related issues:

@alvint
Copy link
Author

alvint commented Dec 2, 2014

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
Copy link
Member

phated commented Dec 2, 2014

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

@alvint
Copy link
Author

alvint commented Dec 2, 2014

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

@alvint
Copy link
Author

alvint commented Dec 2, 2014

...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
Copy link
Member

phated commented Dec 2, 2014

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

@alvint
Copy link
Author

alvint commented Dec 2, 2014

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
Copy link
Author

alvint commented Dec 2, 2014

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

@alvint
Copy link
Author

alvint commented Dec 2, 2014

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

@yocontra
Copy link
Member

yocontra commented Dec 3, 2014

@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

@phated
Copy link
Member

phated commented Dec 8, 2014

I'm going to close this topic as it has already diverged multiple times. I am not going to be working on this feature and if someone wants to contribute actual code instead of bikeshedding it into oblivion and killing my will to work on gulp4, I will happily review the patch.

@phated phated closed this as completed Dec 8, 2014
@geekflyer
Copy link
Contributor

@phated What do you mean with "executing a task" ?
Can't we simply implement a logic for gulp.task, gulp.series and gulp.parallel, so that the arguments (functions) are only executed for --task listing lookup when they are the output of gulp.series or gulp.parallel?

@phated
Copy link
Member

phated commented Dec 8, 2014

@geekflyer i welcome you to read the gulp4 code to understand. feel free to submit a patch

@geekflyer
Copy link
Contributor

Ok, I'll give it a try :-)

@yocontra
Copy link
Member

yocontra commented Dec 9, 2014

I am not going to be working on this feature and if someone wants to contribute actual code instead of bikeshedding it into oblivion and killing my will to work on gulp4, I will happily review the patch.

@phated Please try to maintain an inclusive environment for people who want to contribute.

@phated
Copy link
Member

phated commented Dec 9, 2014

@contra I believe I was being inclusive in saying that I am looking for help in this matter. I won't be implementing this as I don't see any way to do it that makes sense. More bikeshedding without code doesn't move the project along.

@alvint
Copy link
Author

alvint commented Dec 9, 2014

@phated didn't @sttk have code?

@alvint
Copy link
Author

alvint commented Dec 9, 2014

@phated In any case I'll write it. Just give me your requirements (what you wan't the interface and any API contracts to look like).

@phated
Copy link
Member

phated commented Dec 9, 2014

All the code is there and you seem to be defining the requirements.

@alvint
Copy link
Author

alvint commented Dec 9, 2014

@phated You mentioned something about wanting to be able to list dependencies? How do you want that to work exactly?

@phated
Copy link
Member

phated commented Dec 9, 2014

@alvint the gulp --tasks, gulp --tasks-simple and gulp --tasks-json commands all currently work as desired in the 4.0 branch. It needs to continue to work the same.

@phated
Copy link
Member

phated commented Jul 26, 2015

Figured I would update this post. It was easiest to implement forward references as a custom registry - see https://github.com/undertakerjs/undertaker-forward-reference

@mryellow

This comment was marked as abuse.

adericbourg pushed a commit to criteo/graphite-remote-adapter that referenced this issue Sep 12, 2018
gulp 3.9.1 depended on lodash 1.0.1.
Any version of lodash prior to 4.17.5 was impacted by CVE-2018-3721
(Modification of Assumed-Immutable Data).

Fixing it required to upgrade gulp to version 4.0.
File format changed with one significant impact: it does not support
forward references to a task (see
gulpjs/gulp#802). gulpfile.hs has been updated
accordingly.

Signed-off-by: Alban Dericbourg <a.dericbourg@criteo.com>
jeebak added a commit to jeebak/surfingkeys-conf that referenced this issue Dec 27, 2018
jeebak added a commit to jeebak/surfingkeys-conf that referenced this issue Dec 27, 2018
AssertionError [ERR_ASSERTION]: Task function must be specified
AssertionError [ERR_ASSERTION]: Task never defined: readme
  gulpjs/gulp#802
[10:31:47] The following tasks did not complete: check-priv
[10:31:47] Did you forget to signal async completion?
[10:53:00] Error: watching conf.priv.js,completions.js,conf.js,actions.js,help.js,keys.js,util.js,gulpfile.js: watch task has to be a function (optionally generated by using gulp.parallel or gulp.series)
jeebak added a commit to jeebak/surfingkeys-conf that referenced this issue Dec 28, 2018
AssertionError [ERR_ASSERTION]: Task function must be specified
AssertionError [ERR_ASSERTION]: Task never defined: readme
  gulpjs/gulp#802
[10:31:47] The following tasks did not complete: check-priv
[10:31:47] Did you forget to signal async completion?
[10:53:00] Error: watching conf.priv.js,completions.js,conf.js,actions.js,help.js,keys.js,util.js,gulpfile.js: watch task has to be a function (optionally generated by using gulp.parallel or gulp.series)
jeebak added a commit to jeebak/surfingkeys-conf that referenced this issue Dec 28, 2018
Fixes for these gulp errors:
  - AssertionError [ERR_ASSERTION]: Task function must be specified
  - AssertionError [ERR_ASSERTION]: Task never defined: readme
  -   gulpjs/gulp#802
  - The following tasks did not complete: check-priv
    - Did you forget to signal async completion?
  - Error: watching conf.priv.js,completions.js,conf.js,actions.js,help.js,keys.js,util.js,gulpfile.js: watch task has to be a function (optionally generated by using gulp.parallel or gulp.series)

Update README to use local ./node_modules/.bin/gulp instead of global
gulp.
jeebak added a commit to jeebak/surfingkeys-conf that referenced this issue Dec 28, 2018
Fixes for these gulp errors:
  - AssertionError [ERR_ASSERTION]: Task function must be specified
  - AssertionError [ERR_ASSERTION]: Task never defined: readme
  -   gulpjs/gulp#802
  - The following tasks did not complete: check-priv
    - Did you forget to signal async completion?
  - Error: watching conf.priv.js,completions.js,conf.js,actions.js,help.js,keys.js,util.js,gulpfile.js: watch task has to be a function (optionally generated by using gulp.parallel or gulp.series)

Update README to use local ./node_modules/.bin/gulp instead of global
gulp.
@joeyhub

This comment has been minimized.

@gulpjs gulpjs locked and limited conversation to collaborators Mar 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants