timhall commented 6 years ago

Thanks @lukeed and @jbucaran for your work on taskr! I wanted to present an idea I'd had on a potential API change.

Summary

This RFC presents an alternative plugin API that matches the current task API.

Unifying tasks and plugins should encourage task reuse (as plugins), simplify the overall API, and allow a single core mental model for how taskr tasks and plugins function.

Detailed Design

Generally, a unified task or plugin has the following shape:

type Result = void | any | Task;

function (task: Task, ...options: any[]): Result | Promise<Result> {
  // Transform given task with options
}

All tasks and plugins are passed to Bluebird.coroutine, so all function types, including async functions and generators, are supported.
If a Task is returned, taskr will continue work from that task's state, allowing task chains to be created (more details below).
For non-task returns, the current val approach is used, to pass values to subsequent tasks.

To utilize this new approach in a semver-minor-friendly manner, plugins using the unified model are denoted with named exports and use peerDependencies to specify a minimum supported taskr version. This should allow changes while maintaining compatibility with the existing ecosystem.

// Existing approach
module.exports = function(task) {
  task.plugin('name', { files: true }, function * (files) {
    // ...
  });
};

// Unified approach
exports.name = function * (task) {
  const files = task._.files;
  // ...
};

It's fairly straightforward to match existing functionality with wrapper utilities:

// Before
module.exports = function(task) {
  task.plugin('sass', { every: true }, function * (file) {
    // ...
  });
}

// After
const every = require('@taskr/utils').every;

exports.sass = function(task) {
  return every(task, function * (file) {
    // ...
  });
};

To accommodate this new approach, an updated form of task.run is introduced which runs the task/plugin directly.

async function build(task) {
  task.a().b() === task.run(a).run(task => b(task));
  task.start('js') === task.run(js);
  task.parallel(['js', 'css']) === task.parallel([js, css]);
}

(see an example implementation here: example gist)

Finally, to allow chaining, tasks continue from a previous task if a Task is returned.

async function build(task) {
  return task.run(js).target('build');
}

async function js(task) {
  // By returning the resulting task from source -> babel
  // the parent can continue chain
  return task.source('js/**/*').babel();
}

In addition to allowing for chaining, this removes unexpected side effects from tasks run in succession, see #289, by returning a new task from each step rather than mutating the initial task.

async function build(task) {
  await task.a().b().c();
  //         ^   ^   ^
  // a new, lightweight task is generated at each step
  // -> no side-effects on original task

  // task has not been mutated, can safely run subsequent tasks
  await task.d().e().f();
}

Future Possibilities

This new approach allows for many interesting future possibilities, e.g. merging tasks:

async function build(task) {
  return task.merge(js, css).target('build');
}

async function js(task) {
  return task.source('src/js/**/*.js').typescript().babel();
}
async function css(task) {
  return task.source('src/css/**/*.scss').sass();
}

Conditional tasks:

const production = process.env.NODE_ENV === 'production';

function js(task) {
  task.source('src/js/**/*.js')
    .typescript()
    .babel()
    .check(production, task => task.uglify());
}

Drawbacks

While the side-effect free approach of chaining tasks may be preferred for its "correctness", it may cause compatibility issues with existing systems. Task changes will have to be approached carefully.

Alternatives

These examples are possible with the current version of taskr, but they should be much more straightforward to implement with the unified system.

lukeed commented 6 years ago

Wow! Thank you for writing this up! This is the first RFC I've received, and it's pretty awesome 😄

Here are my initial thoughts, I may be wrong or misspeak as I go 😇 But I'll definitely be revisiting this a few times as it sinks in & I can think about it more:

Unifying tasks and plugins should encourage task reuse (as plugins)

This is already easily done, I just haven't been good at pointing it out in the docs and example kits.
```
// taskfile.js
exports.styles = require('@lukeed/sass-task')
exports.scripts = require('@lukeed/babel-task')

// sass-task
module.exports = function * (task, opts) {
  yield task.source(opts.src || 'src/**/*.{sass,scss}').sass({ ... }).target('dist/css');
}
```
Some work could be done on supplying source and target values (eg, globals?), but that applies to your suggestion too.
Your proposed method of chaining tasks (task.run(a).run(task => b(task))) would infinitely nest tasks. For example, the next layers down:
```
task.run(a).run(task => {
  return b(task).run(task => {
    return c(task).run(task => {
      return d(task);
    });
  });
});
```
That's assuming task.run(a) even works as is -- it'd likely need to be a(task) like the others.

That said, your scenario could work if run were a recursive loop, but the main issue is how to set up the loop such that run knows when a new chain-segment is waiting to be run. That's a massive, breaking rewrite on its own.
Thanks for the gist 🙏
That every utility would need to accept a globs vs files modifier.
Pairing 3 & 4 makes me concerned for memory utilization. These two points, alone, add a lot more returned functions to the mix.
By returning the resulting task from source -> babel, the parent can continue chain

This can already be done. All segments of a chain are Promises, as I'm sure you know. A chain can continue after any returned segment, even after a target().
```
const js = task => task.source('js/**/*').babel();

exports.build = function * (task) {
  yield js(task).target('build');
}
```
This also works:
```
export async function js(task) {
  return task.source('js/**/*.js').babel();
}

export async function build(task) {
  await task.start('js').target('dist/js');
}
```
My first example hoisted it to a helper function (instead of an exported task function) because IMO there's no point in exporting the js task since it achieves nothing.
I appreciate the consideration towards #289, but that's not the issue in this case. I've had it on my list for a while, just haven't had time.

New Tasks are returned. The issue is how the Tasks are booted. The boot method writes into the same Promise.prototype, which then gets shared across tasks. This, then, is what "muddies" (as I like to call it) the source info (_) across tasks.

I'm fairly sure I just need to alter the boot method to ignore certain keys (eg _) and/or handle functions only.
I'm not sure I understand the point of merge --- at least, not how it's presented.

Your illustration is just a parallel chain. Nothing is "merging" as I can see.

However, this makes me think of an optimization for my first bulletpoint:
```
// taskfile.js
const sass = require('@lukeed/sass-task')
const babel = require('@lukeed/babel-task')

exports.styles = sass
exports.scripts = babel
exports.build = [sass, babel] //<= "merge"
```
This can be achieved (as presented) just by adding a condition for Array-type tasks. I assume this would be a parallel sequence.
The conditional chains are also easily achievable right now. Even with your syntax, it could be a simple plugin. In fact, I had a taskr-if plugin on my low-priority todo list for a while.
```
taskr.if(condition, isTruthy, isFalsey)
```
As it is, I've taken the lazier route & just run a simple if-block:
```
if (true) {
  yield task.start('foobar');
}
```
Its "cool factor" is lacking, but that's about it -- works well 😆

I played with a similar approach before arriving at Fly 2.0. My "big idea" was to make the Task class completely stateless, and instead be a factory function that runs a function with given values. The problem was that run (and just about everything else) had to retain some knowledge about its values... hence state.

The only form that "worked" massively spiked in memory usage, and Taskr itself added a couple of pounds.

This "final form" has been in production for a year now & has been performing exceptionally well across multiple use cases.

I'm not saying "it will never change!" by any means -- however, I fundamentally disagree with (what I think is) the premise of this RFC:

Plugins and Tasks are not the same thing, by design. Tasks encapsulate plugins, which then allows you to run anything within a Task under any mode or condition. And because of this, Tasks themselves are quickly portable and interchangeable.

Plugins, by design, "globally" add or alter what Tasks can perform. Tasks perform and can tell others how to perform, too.

Gunna interrupt myself here; but the main the point is that Tasks and Plugins are two very distinct vocabularies and therefore deserve slightly distinct definitions. This line in the sand makes & keeps it clear in discerning what-is-what... but it's also only a line in the sand, as opposed to a solid brick wall. There's still plenty of room for flexibility, extensibility, and modularity when it comes to Taskr imo.

As we've both pointed out, everything presented (except merge) can be done today. Any extra convenience layers can be wrapped up in a plugin. And any tasks can be wrapped up & shipped around too.

Please let me know if I missed something important, or just flat-out misunderstood a point. 😆

Thank you again 🙌

timhall commented 6 years ago

Thanks for such a detailed response! It's always nice to get a picture into a project's philosophy, I can definitely see the wisdom in explicitly separating the concerns of tasks and plugins. I'll clean up my thoughts based on your response and dig a little deeper into the source since it seems like a good amount was existing (it may just be a matter of clearing it up in the docs).

timhall commented 6 years ago

After thinking about it more based on your responses, what I was thinking can be boiled down to one suggested change:

Allow function of string option for start, serial, and parallel

This would go a long way to composing tasks without having to export everything including tasks that are not meant to be called directly.

function * build(task) {
  yield task.parallel([js, css]);
}

// "internal" functions 
function * js(task) {}
function * css(task) {}

module.exports = {
  build
};

On the question of merge, it was intended to be used in plugins like rev that need access to the output of many separate tasks (e.g. html, js, and css). The results from multiple tasks are merged into a single files/globs and processing continues. Based on your responses, this should be doable with the current system:

module.exports = {
  name: 'merge',
  every: false,
  *func(files, tasks) {
    // key is `this` = `task` in plugin, so chaining is available with `this.start`
    // (might be able to do this with parallel, not sure what is returned though)
    const results = yield Promise.all(tasks, task => this.start(task));
    for (const result of results) {
      // Merge result._.files/globs into this._.files/globs...
    }
  }
};

Great work on the library, really nice to find out most of what I was looking for was already available!

lukeed commented 6 years ago

Hey, great! That looks like something that can be done 🎉

Do you mind copy/pasting that "pass a function" bit into a new issue? We can close this & it'll be easier for tracking.

Yes, that way of "merging" can be done right now. However, the Promise.all you wrote needs a bit o' fixing:

Promise.all(tasks.map(task => this.start(task));
// or
Promise.all(tasks.map(this.start))

timhall commented 6 years ago

:+1: #292

lukeed / taskr

RFC: Task / Plugin Unification #291

Summary

Detailed Design

Future Possibilities

Drawbacks

Alternatives