lukeed / taskr

A fast, concurrency-focused task automation tool.
MIT License
2.53k stars 74 forks source link

Ability to pass in arguments to `start` and `watch` tasks #187

Closed tjbenton closed 8 years ago

tjbenton commented 8 years ago

It would be nice if you could pass in arguments to tasks when they're called from fly.start and fly.watch

@bucaran Here's an example of how I think it could work

const paths = {
  styles: 'app/lib/styles/**/*',
  js: 'app/lib/js/**/*'
}

export default async function() {
  this.start([
    // yes it's the same as the `styles` task but it's an example.
    // the key value in the object would be the name of the task
    { styles: paths.styles }, 
    // you could still pass in a string for backwards compatibility
    'js'
  ], { parallel: true }) // still pass in options as normal
}

export async function styles(globs) {
  await this
    .source(globs || paths.styles)
    .scss()
    .target(dist)
}

export async function js(globs) {
  await this
    .source(globs || paths.js)
    .babel()
    .target(dist)
}

export async function watch() {
  // `fly.watch` would update to be able to accept a function.
  // This would just be an addition to accepting an `array` not replacing it,
  // this way the changes can be backwards compatible
  const styles = this.watch(paths.styles, async (changed) => {
    // the initial time this function runs the `changed` variable could be
    // the glob that was passed (aka `paths.styles`), then after that `changed` 
    // would be the files that changed. This way users have the option to pass in 
    // just the files that changed, which would save a ton of time on tasks
    await this.start({ styles: changed })
  })
  const js = this.watch(paths.js, async (changed) => {
    await this.start({ js: changed })
  })

  // other watch tasks...

  await Promise.all([ styles, js/*, ...other tasks */ ])
}

The reason why I bring this addition up is because there's several times I've had tasks that are looking at all js files for changes and the js task is clearing the entire js dist directory and then rebuilding it. That becomes quite time consuming when I'm just making a simple change to 1 file and all I really need is to run that 1 file through the task.

lukeed commented 8 years ago

Hmm.. I'm not a big fan of the start changes, nor the idea behind it. I think all tasks should have their source baked in, as a default.

What I do like is the changed handler. I think this should probably be worked into the watch method, and, by default, handle the changed/updated files only. This would also mean that the destination files/folders will only be deleted & overwritten in relation to the changed file.

yield this.watch('scripts/**/*.js', ['scripts']);

// (1) glob default source
exports.scripts = function * () {
  yield this.source('scripts/*.js')
    .babel({}).target('dist/js');
};

// (2) non-glob, single source
exports.scripts = function * () {
  yield this.source('scripts/app.js')
    .browserify().target('dist/js');
};

Changing the file scripts/chat/index.js while using the first scripts function will, behind the scenes, inject the individual file into the function & treat that as the new source. This is because (and only because) the default source is a glob selection; so the new source adds specificity.

The second scripts function will not adopt the new source, because it's already the most specific it can get. This is ideal for tasks that rely on single entry, like browserify, as shown.

What you think?

tjbenton commented 8 years ago

I'm not sure how well that would go over in the long run because you're forcing that functionality upon tasks that might work well with it. Instead of letting the author of the task decide how to handle it.

Here's an example of how I usually end up building out styles. Currently I have site styles and page styles separated out because it was taking too long for everything to compile when I only needed 1 of them to run. However I need to do exactly the same thing to both tasks and that's why I wrote the postcss function at the bottom of the gist. If you could pass in options this.start, it would make this part easier. You're right making and object ({ styles: paths.styles }) and passing it arguments could get messy especially if there're multiple arguments because then you would have to pass in an array. I'd be okay with looking into other ways to use this.start. I know the next version of gulp is using gulp.parallel to run tasks in parallel, and that could be a good way of handling that. Personally I would just use await Promise.all(...) to run things in parallel because it's just as easy, and that's what it's for.

export async default function() {
  await this.clean('dist')
  await Promise.all([ this.start('site'), this.start('js') this.start('images') this.start('etc') ])
  await this.start('server')
}

If you did remove the parallel option from this.start it would make it easier to pass in arguments in a easier way because

export async function siteStyles() {
  ...
  await this.start('postcss', 'dist/styles/site.css', 'dist/styles')
}

1 problem with automatically passing in the file that changes is that you will also have to do the same thing for this.clear because for most tasks that use **/* in this.source are also using the same thing for their this.clear just with a different base directory (aka app vs dist). I'm you could do some logic to determine this to get it to work.

Another problem with it is that the file that changed might have just been to trigger a recompile a different glob of files. For example when app/styles/tools/_variables.scss changes I trigger stylesSite, and stylesPages because both tasks are watching for it to change _variables.scss. If you automatically injected the changed file into stylesSite it would work because of the way you described above because it's source is app/styles/site.scss so the changed file would not be passed in. However stylesPages uses a glob of app/styles/pages/*.scss so If you tried to pass in app/styles/tools/_variables.scss it would not run as expected. For this task I would want to check the path that changed and if it was in app/styles/pages then only run the changed file through it otherwise recompile all of the pages.

I think leaving it up to the developer who's authoring the task to determine what is passed in would be the best option because it's the most flexible.

tjbenton commented 8 years ago

After looking through the source code I discovered that the this.start command actually accepts a value key in it's options and that option is passed into the function that is ultimately called by this.exec. Knowing this if you update the Fly.prototype.watch function to pass in the file that's changed you can get the functionality that allows you to pass in 1 file instead of a glob full of a bunch of files.

Fly.prototype.watch = function (globs, tasks, options) {
    options = options || {}
    globs = arrify(globs)

    _('watch %o', globs)

    var self = this
    return self.emit('fly_watch').start(tasks, options).then(function () {
        return chokidar
            .watch(flatten(globs), {ignoreInitial: true})
            .on('all', function (type, glob) {
                options.value = null
                // pretty print the event type that happened
                type = type.replace(/[a-z]+/, function(match) {
                    return { add: 'Added ', change: 'Changed', unlink: 'Removed ' }[match] || ''
                }).trim()
                self.log(type + ' ' + glob)

                // if a single file was passed, then pass in the file to the tasks
                if (!!path.extname(glob)) {
                    options.value = glob
                }

                return self.start(tasks, options)
            })
            .on('error', self.error)
    })
}
lukeed commented 8 years ago

Closed by #189