Exploring new syntax for defining dependencies #26

Closed
sindresorhus opened this Issue Feb 3, 2014 · 63 comments

Projects

None yet
@sindresorhus

Running tasks concurrently works fine in most cases, but sometime you need to run some tasks after each other. Using task deps works, but can get ugly in some cases.

In this example I want to run the clean task before running the foobar task. I could add clean as a dep to foo and bar, but that gets ugly quickly. If a want to run a task after foobar is done it gets even uglier.

gulp.task('foo');
gulp.task('bar');
gulp.task('foobar', ['foo', 'bar']);

gulp.task('clean');
gulp.task('default', ['clean'] function () {
    gulp.start('foobar');
});

I think Orchestrator should have some built-in sugar for handling this.

Relevant: https://github.com/OverZealous/run-sequence

@contra
Contributor
contra commented Feb 3, 2014

Or a better way to describe sequential dependencies which I think could be easier than run-sequence from a user perspective if done correctly

@contra
Contributor
contra commented Feb 3, 2014

I had an idea for something like this

gulp.task('foo');
gulp.task('bar');
gulp.task('clean');

gulp.task('default', ['clean', ['foo', 'bar']]);

Where nested arrays are new orchestrations

@darsain
darsain commented Feb 3, 2014

That would default to sequential, and defining dependencies that should run concurrently would be a little weird:

gulp.task('default', [['foo', 'bar']]); // yo dawg

How about this?

gulp.task('default', 'clean', ['foo', 'bar'], ['baz', 'bax'], maybeCallback);

Defining sequential dependencies as 2nd...n arguments (n-1 when last is callback).

The current signature would still work as it is now while retaining backwards compatibility, but now you can also define sequential dependencies! :)

@contra
Contributor
contra commented Feb 3, 2014

@darsain The behavior would only occur with nested arrays. The normal stuff right now (with max concurrency) would still work fine. Nesting arrays would create different orchestrations that run in order.

@robrich
Owner
robrich commented Feb 3, 2014

This syntax seems ugly. It's not obvious where defining the task ends and
starting the orchestration begins, and it's not obvious when a new
orchestration is created. [ isn't very obvious to the unlearned.

@contra
Contributor
contra commented Feb 3, 2014

@robrich Just an idea. I'm sure we can think up an elegant API to solve this

@heikki
heikki commented Feb 3, 2014

Idea:

gulp.task('assets', function() {});
gulp.task('scripts', function() {});
gulp.task('styles', function() {});
gulp.task('views', function() {});
gulp.task('clean', function() {});
gulp.task('dev', function() {});

gulp.parallel('build', ['assets', 'scripts', 'styles', 'views']);
gulp.series('default', ['clean', 'build', 'dev']);

Remove task dependencies. Add parallel and series for composing.

@heikki
heikki commented Feb 3, 2014

Variation:

function assets() {}
function scripts() {}
function styles() {}
function views() {}
function dev() {}

gulp.task('clean', function() {});
gulp.parallel('build', [assets, scripts, styles, views]);
gulp.series('default', ['clean', 'build', dev]);

Use task, parallel and seriesonly for those parts that need to be exposed to cli.

@phated
Contributor
phated commented Feb 4, 2014

This is only desirable for people that don't understand dependencies. I can see what @sindresorhus is saying about it getting ugly quickly, but also, he isn't being DRY about it.

var deps = ['clean'];

gulp.task('foo', deps);
gulp.task('bar', deps);
gulp.task('clean');

gulp.task('foobar', ['foo', 'bar']);
gulp.task('foobar2', deps, function(){
    console.log('clean has run');
});

gulp.task('default', ['foobar']);

Sequential running doesn't make any sense in gulp/orchestrator because you shouldn't register tasks that won't be run from the command line. They should just be functions.

@contra
Contributor
contra commented Feb 4, 2014

@phated The goal here is to solve this problem without

a) creating dummy tasks to do the dependencies
b) using gulp.start
c) having to add clean as a dep to every task in the file

Also your example code wouldn't work since gulp.task('foobar', deps, ['foo', 'bar']); is not a supported API

@phated
Contributor
phated commented Feb 4, 2014

@Contra I updated the example. Not having people specify which tasks need clean will just lead to more problems. Not all tasks need to clean and letting people specify some things in sequence and some async is going to lead to more headaches to manage.

Nine times out of 10, people asking for this aren't thinking in a gulp style and want it to behave like grunt because that is where their mind has been at for so long. This isn't a reason to screw with the API.

@contra
Contributor
contra commented Feb 4, 2014

@phated As the gulp BDFL I don't think this is an infringement on "gulp style". Sugar for defining dependencies doesn't have anything to do with grunt or grunt behavior. Grunt doesn't even have a task system or dependencies so how did you reach that conclusion? Don't be so quick to shut down new ideas.

@contra
Contributor
contra commented Feb 4, 2014

@phated Grunt figures out the best way to run tasks based on grunt.config.requires? I wasn't aware. Is that something a user defines or a plugin?

Looks to me like something for validating config files from first glance

@phated
Contributor
phated commented Feb 4, 2014

Grunt doesn't do the ordering because it runs everything sync. Requires makes the task fail if something hasn't happened in the right order. Gulp is the opposite because it runs everything in the correct order, this requires a little extra verbosity because it allows for extra flexibility. Two different people created grunt async userland stuff (I believe @sindresorhus was one of them) because running in sync is slow and generally makes tasks highly coupled (validation!?). Why is it bad/wrong to push "sync" behavior to userland since it can be done with sugar vs adding complexity to orchestrator (which is already too complex).

@contra
Contributor
contra commented Feb 4, 2014

@phated You misunderstand my goals here. I don't want to change the way anything works, I want to make it easier to follow the correct behavior.

@phated
Contributor
phated commented Feb 4, 2014

@Contra I think we differ in best behavior. I think the best behavior is to use dependencies on tasks since tasks should be run at the command line and use functions for things that aren't run at command line.

@contra
Contributor
contra commented Feb 4, 2014

@phated No we don't. I agree with you, but I think we can improve the syntax for defining dependencies to make this use case (running tasks in sequence) easier and more intuitive.

@darsain
darsain commented Feb 4, 2014

@phated I want sequential dependencies precisely so I can be DRY. Here is an example gulpfile:

var gulp = require('gulp');
var clean = require('gulp-clean');
var stylus = require('gulp-stylus');
var browserify = require('gulp-browserify');

gulp.task('clean', function () {
    return gulp.src('build', { read: false }).pipe(clean());
});

gulp.task('scripts', function () {
    return gulp.src('js/main.js').pipe(browserify()).pipe(gulp.dest('build'));
});

gulp.task('styles', function () {
    return gulp.src('styl/main.styl').pipe(stylus()).pipe(gulp.dest('build'));
});

gulp.task('build', 'clean', ['scripts', 'styles']);

gulp.task('default', 'build', function () {
    gulp.watch('styl/**/*.styl', ['styles']);
    gulp.watch('js/**/*.js', ['scripts']);
});

All tasks should be able to be run from command line. scripts & styles should be decoupled to easily specify watchers.

In build, clean has to run before scripts & styles. How do you do that right now without gulp.start()? Or without duplicating some logic into multiple tasks? You can't add clean as a dependency for scripts & styles because when any of their watchers kicks in, it destroys other task's output.

And look how comfy the example above is. Don't make it more complicated! :)

@phated
Contributor
phated commented Feb 4, 2014

@darsain I fixed it for you with the current syntax.

var gulp = require('gulp');
var clean = require('gulp-clean');
var stylus = require('gulp-stylus');
var browserify = require('gulp-browserify');

var deps = ['clean'];

function clean () {
    return gulp.src('build', { read: false }).pipe(clean());
}

function scripts () {
    return gulp.src('js/main.js').pipe(browserify()).pipe(gulp.dest('build'));
}

function styles (){
    return gulp.src('styl/main.styl').pipe(stylus()).pipe(gulp.dest('build'));
}

gulp.task('clean', clean);
gulp.task('scripts', deps, scripts);
gulp.task('styles', deps, styles);

gulp.task('build', ['scripts', 'styles']);

gulp.task('default', ['build'], function () {
    gulp.watch('styl/**/*.styl', styles);
    gulp.watch('js/**/*.js', scripts);
});
@contra
Contributor
contra commented Feb 4, 2014

What about making something for lazily adding deps?

@phated
Contributor
phated commented Feb 4, 2014

@Contra don't we already have that? Orchestrator doesn't do a copy on an array, you can just mutate it.

@contra
Contributor
contra commented Feb 4, 2014

@phated sure but it isn't pretty

var excluded = ["clean", "default"];

Object.keys(gulp.tasks).filter(function(k){
  return excluded.indexOf(k) !== 1;
})
.map(function(k){ return gulp.tasks[k]; })
.forEach(function(v){
  v.deps.push("clean");
});

To say "add clean as a dep to all tasks but default and clean itself"

@contra
Contributor
contra commented Feb 4, 2014

Or if you knew all of the task names

var tasks = ["foo", "bar"];

tasks.forEach(function(v){
  gulp.tasks[v].deps.push("clean");
});
@phated
Contributor
phated commented Feb 4, 2014

@Contra there is almost no situation where clean will be run before every task. The 2nd solution covers 99% of the issues and works pretty well. It can be abstracted into gulp or userland, but shouldn't be an orchestrator concern.

@contra
Contributor
contra commented Feb 4, 2014

@phated I run clean before every single task in almost all of my gulpfiles, I'd say it's pretty common. Even if orchestrator just exposed a simple bulk add dep function that sugars some of this away it would make the problem a lot easier to solve

@phated
Contributor
phated commented Feb 4, 2014

@Contra I think that an gulp.order should be an abstraction on the code in your 2nd example.

@darsain
darsain commented Feb 4, 2014

@phated (regarding your last example) I'm doing the exact same thing in my current gulpfiles - functions returning streams which I than recycle throughout tasks. I've considered it ugly & redundant as the only reason why I'm doing it is the lack of easy defining of sequential dependencies. But if this is the consensus, I'm gonna conform.

It still seems silly to me though.

@phated
Contributor
phated commented Feb 4, 2014

@darsain there is even a better way to do it, but I didn't want to stray too far from your original example. The better way would allow for sequential updates.

@robrich
Owner
robrich commented Feb 5, 2014

Wow, this discussion has gotten out of hand. :D Some points of clarification:

  • the orchestrator rewrite does copy the dependency array. (In hind-sight, exposing the list of tasks to user-land was probably a bad idea, but I haven't found a good way to pull it back yet.)
  • #27 has some interesting commentary on making both before dependencies and after dependencies (e.g. "I always want to run reload after jsmin finishes").
  • The major topic of this issue seems to be the desire to sometimes specify dependencies when creating the task, and sometimes when building the orchestration. (e.g. orchestrator.run() or .start().) I see value in this, though I've yet to settle on the best syntax for this.
@robrich robrich added the enhancement label Feb 5, 2014
@phated
Contributor
phated commented Feb 5, 2014

@robrich I point to this as yet another reason you should be working on code in a branch in the open :(

@robrich
Owner
robrich commented Feb 5, 2014

@phated +1 :D

@darsain darsain referenced this issue in gulpjs/gulp Feb 5, 2014
Closed

Run task in series more elegantly #242

@vohof
vohof commented Feb 6, 2014

+1 at heikki's suggestion

@scriby
scriby commented Feb 13, 2014

I too would like to see a way to be able to run a string of tasks in series, which would allow for composing existing tasks in new ways. See gulpjs/gulp#265 for specifics of my use case.

@Anachron

Honestly, I think gulp just got ugly by not allowing to use the "run" command.
Here is what I had in mind:

  # Saving
  gulp.task 'saveStyle', ->

  gulp.task 'saveScript', ->

  gulp.task 'saveTemplate', ->

  gulp.task 'beforeSave', (type) ->

  gulp.task 'afterSave', (type) ->

  gulp.task 'save', ->
    type = getTypeOfThisStreamSrc
    gulp.run 'beforeSave', type
    switch type:
      when 'style' then gulp.run 'saveStyles'
      when 'script' then gulp.run 'saveScript'
      when 'template' then gulp.run 'saveTemplate'
    else
      # Something is wrong
    gulp.run 'afterSave', type

  # Later user will inject his stuff

  awesomeLib.inject 'beforeSave', 'style', (type) ->
    console.log 'A ' + type + ' got catched!'
@contra
Contributor
contra commented Feb 15, 2014

@Anachron use gulp.start if you absolutely feel you must. gulp.run was removed because it was a worthless alias.

Not sure why people keep crying about removing an alias, maybe we didn't document the change well enough

@sindresorhus

maybe we didn't document the change well enough

Yup

@Anachron

Thanks man, that makes a lot of sense! =)
Will be creating my gulp-wrapper now.

@contra contra referenced this issue in gulpjs/gulp Feb 16, 2014
Closed

Better style of gulp tasks definition #273

@robrich
Owner
robrich commented Feb 18, 2014

I just found an awkward scenario: how do we tell the difference between a series and a parallel dependency chain? Getting them to execute internally is cake, but ensuring a simple and discoverable API is where we're stumbling.

How would you specify each of these examples?:

Example One: task1 and task2 and task3 can run in parallel and after all 3 of those are done, run task_a, and when that's done, run task_b, and tell me when everything is done.

Example Two: run task1 then task2 then task3 in series, and while running those, also simultaneously (in parallel) run task_a and also simultaneously run task_b and tell me when everything is done.

How would each of these be identified in the new nested array syntax and/or do we need to find a different syntax?

@contra
Contributor
contra commented Feb 18, 2014

Idea that me and robrich just discussed: A fluent API for creating orchestrations.

var tests = orchestrator.series('stuff', 'stuff2');
var deploy = orchestrator.parallel(
  orchestrator.series('stuff3', 'stuff4'),
  orchestrator.series('stuff5', 'stuff6')
);
var buildStuff = orchestrator.parallel('something', 'something2', tests);
orchestator.start(buildStuff);

You would be able to nest/combine these infinitely. Just a thought, the syntax could use some work to prevent huge one liners.

@phated
Contributor
phated commented Feb 18, 2014

๐Ÿ‘ from me. I was working on this concept in my head this morning.

@tkellen
tkellen commented Feb 18, 2014

๐Ÿ‘ this will simplify my integration of orchestrator for grunt-next in a huge way.

@robrich
Owner
robrich commented Feb 18, 2014

I like this thought, and it clarifies the difference between series and parallel without awkward array stuff. Nice!

orchestrator.series(
  'stuff1',
  'stuff2',
  orchestrator.parallel('stuff3', 'stuff4'),
  'deploy'
).run();

What do you think of this for a fluent syntax:

orchestrator.series('stuff1','stuff2')
    .appendParallel('stuff3','stuff4')
    .pop()
    .afterSeries('stuff5')
    .run()
@phated
Contributor
phated commented Feb 18, 2014

I dislike the fluent syntax

@tkellen
tkellen commented Feb 18, 2014

seconded.

@sindresorhus

Idea that me and robrich just discussed: A fluent API for creating orchestrations.

Huge ๐Ÿ‘

@jasonrhodes

Could you borrow from promise language here? Both "series" and "parallel" return a promise with a then() method to chain task groups together...

On Feb 18, 2014, at 2:51 PM, Sindre Sorhus notifications@github.com wrote:

Idea that me and robrich just discussed: A fluent API for creating orchestrations.

Huge

โ€”
Reply to this email directly or view it on GitHub.

@contra
Contributor
contra commented Feb 19, 2014

@jasonrhodes no i hate promises

@contra
Contributor
contra commented Feb 19, 2014

Okay so I think we are agreed that the API for building the dependencies on the fly is pretty cool right? @robrich said it would not be too hard to implement this in the new orchestrator branch

@sindresorhus

Yes

@Anachron

You could mix async with promises and build a dependency tree/circle/graph/anything with that.

@robrich
Owner
robrich commented Feb 24, 2014

What if Orchestrator.run() was removed completely, and instead .run() was a function of the builders (series and parallel)? Then there's no ambiguity of whether you wanted to run sequentially or run in maximum concurrency.

var o = new Orchestrator();
o.task('bla', function (cb) { cb(null); });
o.task('de', function (cb) { cb(null); });
o.task('blah', function (cb) { cb(null); });

o.parallel('bla', 'de', 'blah').run(function (err, stats) {
  console.log('done');
});

We could then create sugar methods for running a series and a parallel in one call:

o.runParallel('bla', 'de', 'blah', function (err, stats) {
  console.log('done');
});

This ensures anything currently calling .start() (and gulp.run()) will break. (see #36 (comment))

@phated
Contributor
phated commented Feb 25, 2014

I kind of like that the builders would have the run function, instead of orchestrator instance having it.

@contra
Contributor
contra commented Feb 25, 2014

I also like moving the run function to the tree instances (builders) vs. having it on the orchestrator instance.

So with this syntax we would completely remove dependencies from task declarations and instead do them at run time?

@phated
Contributor
phated commented Feb 25, 2014

It seems that way, but I think that we should still allow it to be done at task declaration time if that is still possible (as that is the way I prefer to declare them).

@contra
Contributor
contra commented Feb 25, 2014

@phated I think if we are going to do the builder syntax we may as well go all in. Dependencies at declaration time might be a good idea for backwards compat

@robrich
Owner
robrich commented Feb 25, 2014

I say you can do dependencies either way.

@contra
Contributor
contra commented Feb 25, 2014

@robrich More choices isn't usually better when it comes to API design. This will cause confusion down the road

@robrich
Owner
robrich commented Feb 25, 2014

Most of the time, dependencies are resolvable at creation time. Rare is the
case where runtime must augment it. Removing creation time to satisfy the
edge case (even when compelling) seems a mistake. I grant run time could
define everything but I'd argue it won't come out simpler.

@phated
Contributor
phated commented Feb 25, 2014

I would argue that run time is better for task "aliases" (ala grunt) and creation time is better when you want to run individual tasks from the command line (this case wouldn't really allow for parallel/sequence calls).

@robrich
Owner
robrich commented Feb 27, 2014

The more I think about moving .run() to the builder, the more I'm disliking it. It would be the only method that was chainable. I do like the idea of .run() only taking a builder though, and if you pass strings, it warns for a version or two and then calls .runParallel() with your array.

@rvangundy rvangundy referenced this issue in stevelacy/gulp-bump Mar 16, 2014
Closed

Feature Request: Git Bump #13

@robrich
Owner
robrich commented Mar 16, 2014

This is now a feature of Orchestrator's develop branch. Give it a try and see if it's sufficient.

@robrich robrich closed this Mar 24, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment