lukeed / taskr

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

Add boot-time "register" add-ons config #241

Closed jorgebucaran closed 6 years ago

jorgebucaran commented 7 years ago

First of all thank you everyone for their contributions since the project's release almost 2 years ago. Special thanks to @lukeed for all your hard work last year. You are now an owner. 👏

I will use this thread to post ideas of how I hope fly's API can evolve towards a future release.

Here is some preliminary thoughts, but first a code sampler.

export default {
    model: {
        scripts: ["src/**/*.js", "!src/ignore/**/*.js"]
    },
    tasks: {
        default: async (fly, model) => await fly.watch(model.scripts)
        build: async (fly, model) => {
            await fly
                .source(model.scripts)
                .eslint({/*...*/})
            await fly
                .source(model.scripts)
                .babel({/*...*/}).concat("app.js").target("dist")
        }
    }
}
lukeed commented 7 years ago

Thank you very much @jbucaran! 🎉 Very much appreciated! 😀

Moving to your idea:

I feel that the model you speak of should be handled internally, which, I think, was originally the purpose of the _ key. The "model" itself should be the product of reducers & never altered directly.

I started this in the beta's development by continuing to keep track of all globs and files internally, but then introduced tracking each file's data as a Buffer. This way, every plugin or inline function is really a reducer on that data and file information & then passes it along to the next method.

What else do you have in mind for a model? Because, unfortunately, I find adding complexity (anything beyond glob patterns) to your example to be more cluttered and problematic. The flyfile, imo, should only house tasks, sources, and any native API implementations.

Your current example could easily be rewritten as:

const src = {
  scripts: ['src/**/*.js', '!src/ignore/**/*.js']
}

export default async function (fly) {
  await fly.watch(src.scripts, ['build'])
}

export async function build (fly, opts) {
  await fly.source(opts.src || src.scripts)
    .eslint({}).babel({}).concat('api.js').target('dist')
}

Worth noting, I do agree that old this should be replace with fly.

It's both simpler, a bit more declarative, and reusable like this.

Keeping the tasks & flyfile this way also allows for incredibly easy and super shareable build environments and tasks. I've done this a few times myself:

// tasks.js
export default async function (fly) {}
export async function build (fly) {}
export async function styles (fly) {}

// flyfile.js
const src = {
  scripts: '...',
  styles: '...'
}

export * from './path/to/tasks'

And via programmatic usage:

import Fly from 'fly'
import tasks from './path/to/tasks'

const fly = new Fly({tasks})
yield fly.start('styles');

Fly's main benefits & core values, imo, are speed, simplicity, reusability & extensibility. From there, anyone can jump in and immediately adjust Fly to their needs. We just have to worry about a great core; the rest will follow.

(I also have other ideas, but it's late and can't recall everything right now. 😪)

jorgebucaran commented 7 years ago

Thanks for replying @lukeed! I've just started to toy with these ideas, so everything is subject to change, but I really appreciate your honest input.

One thing I'd like to do, is try porting some of the ideas from the SAM pattern, into a build system like fly.

I feel that the model you speak of should be handled internally, which, I think, was originally the purpose of the _ key. The "model" itself should be the product of reducers & never altered directly.

Yes, indeed. The model should never be altered directly, that's what reducers, and to some extent effects, are for.

While reducers are sync and update the model directly, effects are often async and may cause side effects. Often they will finish off by sending an action that invokes a reducer, causing the model to be updated as well.

I don't know whether these ideas are reconcilable with a build system, but that's what I'd like to explore.

lukeed commented 7 years ago

Another idea, to avoid saving directly to any internals, is to drop the _ key altogether!

Then every inline & plugin is required to take the chosen source, apply an effect, and then return the effect. Only fly.$ (our new utils object) should be bound to each plugin; otherwise they're completely detached.

// plugin
module.exports = function (fly) {
  fly.plugin('plugName', {options}, function * (src, opts) {
    // `src` defined by `options.every` and `options.files`
    // >> is given either a glob or a file, or all of one

    // Handle plugin options (`opts`)
    const out = api(src, opts);

    // return the modified `src` for next step
    return out
  });
}

That would also mean that fly.source starts the chain by returning an object instead of saving it internally:

// ...
return {
  // update known globs
  globs: globs,
  // update known files, as (mod'd) `pathObject`s
  files: files.map((el, idx) => {
    const obj = parser(el);
    obj.data = datas[idx];
    obj.dir = norm(obj.dir);
    delete obj.root; // use `dir` instead
    delete obj.name; // use `base` instead
    delete obj.ext; // use `base` instead
    return obj;
  })
}

And fly.target should be fly.prototype.target(dirs, opts, files).

Then each task looks like this:

export default async build (fly, opts) {
  await this.source('src/scripts/*')
      // 1. expand globs
      // 2. read files
      // 3. construct initial `source` object
    .eslint({})
      // 1. author set `opts` to `{every: 1, files: 1}`
      // 2. receives every file
      // 3. lints & reports on `file.data`
      // 4. returns `src` (original, no change)
    .babel({})
      // 1. author set `opts` to `{every: 1, files: 1}`
      // 2. receives every file
      // 3. compiles every file
      // 4. returns `output` (has changes)
    .concat('app.js')
      // 1. author set `opts` to `{every: 0, files: 1}`
      // 2. receives ALL source files
      // 3. returns array of one file object
    .target('dist');
      // 1. `dirs` = 'dist'; `opts` = {}; `files` = [{base: 'all', ext: 'js', ...}]
      // 2. continues current `target` behavior
}

I think this will work 😸 It also solves #229 (source parallelism). I think that's the game plan, now just a matter of implementing it. Tomorrow I will give it a go.

jorgebucaran commented 7 years ago

I'm not a huge fan of $ tho.

I'll keep this thread updated with my thoughts on the future. I know it may lead to nothing, but it's always a good idea to experiment with ideas.

lukeed commented 7 years ago

I'm not a huge fan of $ tho.

It's traditionally a "util" shorthand. Plus, assembling a smaller object of helper methods is much much better than attaching directly to the base class. I have future plans to rework the Fly class entirely so that there is less than 8 keys on the entire object -- best for V8.

jorgebucaran commented 7 years ago

The $ is fine. Not a huge fan of it, but I can't think of a way to get rid of it either, so nevermind that 😄

jorgebucaran commented 7 years ago

Here is a bit more of toying with the flyfile:

module.exports = {
    model: ["src/**/*.js", "!src/ignore/**/*.js"],
    watch: ["build"],
    tasks: {
        build: function* (fly) {
            yield fly.eslint({ ... })
            yield fly.babel({ ... }).concat("app.js").target("dist")
        }
    }
}
lukeed commented 7 years ago

I'm sorry 😥 I really don't like that as a base solution because it loses so much flexibility. Personally, I wouldn't use it as it adds duplicate structure to what I already have & it's not as approachable or flexible for my cases.

However!

I think that'd be a great decorator extension for Fly. Because of how versatile the current setup is, this extension is essentially going to do this:

// node_modules/fly-decorator/index.js

const Fly = require('fly');
const config = require('./user/flyfile.js');

config.tasks.watch = function * (_fly, opts) {
  yield _fly.source(opts.src || config.model, config.watch);
}

// return the Fly object, or initialize a task. up to you here
const fly = new Fly({
  tasks: config.tasks,
  file: './user/flyfile.js' // will be sibling to `package.json`
});

await fly.start('watch'); // or whatever

This assumes that you want to watch all model globs.

The beauty of the current setup is that the setup you're after is easily achievable and repeatable!

Also, you need to lead a chain with source. This allows users to define multiple steps within a single task. Imagine if your build task were to handle SASS; you can't force or assume that model can be applied to everything.

lukeed commented 7 years ago

Looking at Webpack: Keeping the core simple & extensible is what allowed for it to be picked up so easily. Many libraries (including Next.js) have been able to add their own decorators directly to Webpack (in a similar fashion) so that they could structure Webpack in a way that makes most sense for them/their users.

jorgebucaran commented 7 years ago

@lukeed Yes, you are right. Again, I'm just playing with ideas. One more thing, what about simply exporting an array with the tasks to run when the watch triggers in the exported default object?

lukeed commented 7 years ago

I know you are; just conversing too. 😄

when the watch triggers in the exported default object?

What do you mean?

jorgebucaran commented 7 years ago

Instead of

x.watch = function * () {
  yield this.watch(paths.scripts, 'build')
}
export default = {
    ...
    watch: ["build"]
    ...
}
lukeed commented 7 years ago

You'd need to define what to watch. The build task doesn't execute ahead of time, so there's no way to know what it's inline source is.

The shortest you could make a watch would be:

// is a Map
exports.watch = [
  [src.styles, 'styles'],
  [src.scripts, ['scripts', 'uglify']]
];

Or you could use an Object{src: tasks}, but then you couldn't provide multiple sources.

Even so, I don't think this is enough of a "timesaver" to make it worth it.

lukeed commented 7 years ago

Also, sidenote: What do you think of extracting fly.watch to its own module? This way everyone doesn't have to suffer the weight of chokidar. Instead, only people who actually use fly.watch will suffer the lag time.

CC @hzlmn @devmondo @watilde

jorgebucaran commented 7 years ago

The same could be said about concat then.

lukeed commented 7 years ago

It's already moved to a plugin :)

jorgebucaran commented 7 years ago

@lukeed Right. I had forgotten about it. Fly has been such a naughty child, now with will of its own and everything 😄

lukeed commented 7 years ago

Fly.prototype.watch is a little bit different because it still described how to handle the defined tasks.

Now it's just a matter if it's a core behavior (where everyone must download Chokidar) or an opt-in behavior.

Everything in Fly.prototype affects either how tasks are run (parallel, serial) or allows / helps tasks to be run (start, source, target, plugin, run).

This actually leaves clear as a black sheep. So this, too, would/could be moved to a plugin.

jorgebucaran commented 7 years ago

I can see that happening. I don't hold strong beliefs wrt this anymore. Before, I fancied how fly was batteries included, but I agree installing chokidar is also a PITA.

lukeed commented 7 years ago

Spent some time & went through this thread once again. 👓

I still believe all ideas presented here are absolutely achievable as plugins/decorators to our current API. Hypothetically, if the roles were switched (object model was current), we wouldn't be able to decorate with the current exported-function syntax.

Put differently, we can define a plugin, install it, and then have Taskr honor any registered taskfile modifiers. These registrations would most likely need to be within "taskr" config (inside package.json). We cold then load @taskr/esnext like this, instead of hard-coding its effect directly.

// package.json
{
  "name": "example",
  "devDependencies": {
    // ...
  },
  "taskr": {
    "require": [
      "./build/custom-plugin-one.js"
    ],
    "register": [
      "@taskr/esnext"
    ]
  }
}

If people like this idea, let me know 👍 I've not yet heard any requests, but that's probably because I did a poor job or saying it's possible!

jorgebucaran commented 6 years ago

@lukeed Maybe this is no longer relevant? We are not using this anymore and like you said:

What else do you have in mind for a model? Because, unfortunately, I find adding complexity (anything beyond glob patterns) to your example to be more cluttered and problematic.

You are probably right, so feel free to close here as well! 👍

lukeed commented 6 years ago

Agreed. I still believe that this is, by definition, a plugin behavior. Definitely still possible though~!