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

Add new rfc for yarn tasks #109

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kaylie-alexa
Copy link
Member

Copy link

@benmvp benmvp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great! 🎉

Looking forward to see how this all shakes out

Feature Requirements:

- Be able to run multiple commands in parallel across platform
- Show and differentiate console output for each command
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is key. The hardest part of scripts ran in parallel is debugging the log later. "Which script generated the error and when?"


## Additional flags

`--hide-name`: By default, tasks will run by appending the name of the command to each line. Enabling this flag would allow it to be hidden from the output.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

appending or prepending? I'm imagining something like:

[build] Webpack is starting to compile...
[lint] No errors found.

Similar to the output of Jest when you use multiple projects

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prepending! nice catch.


# Unresolved questions

- Order of operations for pre/post hooks for installs if they are defined in tasks. Should they happen in parallel or serial?
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would assume serial since that's basically the point. You want the pre hook to happen before the script, not at the same time.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking of a case like "verify": ["**lint"], if there is a prelint and postlint script. Here user intent is a little unclear and also, not all users are aware of the run hooks. I'm leaning towards just removing pre/post from the matched patterns, since they can explicitly state ["**lint", "postlint"] if they really want to run it in parallel.

Third-party libraries, which are additional dependencies and can require complex command line args
Third-party task runners, which have issues mentioned above, mostly due to dependence on ecosystem/maintainers of plugins

# Unresolved questions
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can another unresolved question be the name of the key? Is "tasks" the right name? Hopefully there aren't any prominent libraries using "tasks" already for something else.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, open to any suggestions!

"test:integration": "jest __integration__"
}
"tasks": {
"verify": ["lint", "test**"]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be tempted to drop the globbing feature. Manually specifying each script is clearer and I wouldn't expect these definitions to change frequently or be overly complex.

The feature could be added in a later revision if there was demand for it from the community.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree for a first version it would be simpler not to have globbing

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think it's wise to start w/o the glob too. Solves one of the "unresolved questions" below

"test": "jest",
}
"tasks": {
"verify": ["lint", "test"],

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One slight worry is that lint-staged also uses an array to specify git pre-commit tasks but they use it to mean serial semantics.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could make the tasks an object instead of an array to signify that there's no defined order, but that would be a pain to write.

How big of a deal do you think the issue with lint-staged is?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that an object would be a pain. The lint-staged issue is something that can be handled by documentation so I don't see it as a blocking issue. Of course, there's nothing to say that lint-staged couldn't/shouldn't support parallel tasks and they may adopt whatever syntax yarn chooses.


`yarn run dev` would run lint, test, and build in parallel

_NOTE_ Nested arrays or serial runs will not be supported in tasks because they can be done via scripts using `&&` operator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps this is a horrible (and confusing) suggestion... what if nested arrays was how it all worked? Something like:

{
  "scripts": {
    "clean": "rimraf ./build",
    "build": "webpack",
    "format": "prettier --write .",
    "lint": "eslint .",
    "test": "jest"
  },
  "tasks": {
    "verify": ["format", "lint"], // <-- Runs "format" and "lint" serially
    "pre-commit": [["verify", "test"]] // <-- Runs "verify" and "test" in parallel
    "dev": ["clean", ["build", "test --watch"]] // <-- Runs "clean", then runs "build" and "test --watch" in parallel
  }
}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a bit confusing (lol) but it's not a horrible suggestion. I like it because it broadens the scope of tasks to also solve the problem of just wanting to run tasks serially without having to do yarn lint && yarn test. It helps keep the runner (Yarn) out of the scripts more.

The nested array syntax is a bit funky, but we do see it Babel configurations. Not sure if that helps or hurts the argument though.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking down this path further, perhaps tasks and scripts shouldn't co-exist. If you're wanting to use tasks then it should all be defined there. Formatting can also help show a serial path and parallel tracks:

{
  "tasks": {
    "verify": [
        "prettier --write .",
        "eslint ."
    ],
    "pre-commit": [
        ["verify", "jest"]
    ],
    "dev": [
        "rimraf ./build",
        ["webpack", "jest --watch"]
    ]
  }
}

@arcanis
Copy link
Member

arcanis commented Nov 22, 2018

@kaylie-alexa As mentioned I have a separate RFC in mind that I think might compete in some aspects with this one - do you mind if I open it so we can discuss both at the same time?

As it is the main concern I have regarding this feature is that maybe it isn't "powerful enough", in that it solves one specific use case (immediate parallel execution of a set of scripts) with little room for improvements. For example, the following isn't possible with this model without indirections: (lint & typecheck) && build.

@cameronhunter
Copy link

For example, the following isn't possible with this model without indirections: (lint & typecheck) && build.

I think this would be possible with the nested array syntax I proposed above:

{
  "tasks": {
    "build": [["eslint .", "typecheck"], "webpack"]
  }
}

@rhdeck
Copy link

rhdeck commented Dec 4, 2018

Hi, is there a yarn-compatible utility that take care of this? eg a yarn run tasks <thistaskglob> [...otherargs]? Seems like a good pathway would be to introduce the workflow outside of yarn to validate the use case before modifying core. Any tasks-aware package would include yarn-tasks (or whatever) as a devDependency. Seems like a way to de-risk the proposal and create value sooner. (And have the side-impact of likely working with npm too!)

For my part, this kind of more sophisticated, articulated workflow seems like a great step forward.

(Am a bit new to bigger OSS projects, so would welcome being schooled on why this is a bad idea)

@arcanis arcanis mentioned this pull request Feb 13, 2019
@magicmark
Copy link

@kaylie-alexa @benmvp would it make sense to marry this somehow with #107 ? What are you thoughts here? This implies increased scope for this RFC, but I'm sorta interested in picking this back up, and I don't want to clash with other similar efforts. Are you still interested in something like this? Would be good to chat to see if we might be able to work together!

Cheers!

@ianstormtaylor
Copy link
Contributor

ianstormtaylor commented Oct 8, 2019

Hey folks, love the idea for this!

I was thinking about trying to solve for similar use cases, and had an alternate way to potentially solve this same thing. I figured I'd submit it here in case you think it is a good simplification.

Instead of needing to introduce an extra pkg.tasks, which then might cause confusion as to the differences from pkg.scripts. What if we could solve all of these requirements by just add glob'ing to the existing yarn run command...


Running a single task:

yarn run lint

This already works.


Running two tasks in series:

yarn run lint && yarn run test

This already works too.


Now, introducing globs, we can run two tasks in parallel as:

yarn run '{lint,test}'

This would expand into matching both lint and test.

It also allows for a common use case where people want to run all the commands with a specific prefix, for example test:...

yarn run 'test:*'

This would run any command prefixed with test: in parallel.

Any time a glob is passed to yarn run it will execute tasks in parallel.

(You could argue down the road for a --series flag, but at that point I think it's better to just write out the steps as a separate script target for clarity and clearly defined ordering.)


This also solves nicely for the background scripts use case mentioned in #107, since commands that don't exit would just continue running until you killed them.


Going a bit further, right now you can omit the run, which is convenient:

yarn lint && yarn test

This runs lint and then test in series.

I'm unsure of the implications (so it could be not feasible), but you could potentially do the same with globs for running tests in parallel:

yarn 'test:*'

This would run all test:-prefixed commands in parallel.


Here's an example of scripts from a project of mine:

{
  "scripts": {
    "build": "yarn build:es && yarn build:cjs && yarn build:max && yarn build:min && yarn build:types && yarn build:docs",
    "build:cjs": "rollup --config ./config/rollup-cjs.js",
    "build:docs": "typedoc ./src/index.ts",
    "build:es": "rollup --config ./config/rollup.js",
    "build:max": "rollup --config ./config/rollup-umd.js",
    "build:min": "rollup --config ./config/rollup-umd-min.js",
    "build:types": "tsc --emitDeclarationOnly --declarationMap",
    "check": "yarn lint && yarn test",
    "clean": "rm -rf ./{lib,umd,node_modules}",
    "fix": "yarn fix:prettier",
    "fix:prettier": "prettier --write '**/*.{js,json,md}'",
    "lint": "yarn lint:eslint && yarn lint:prettier",
    "lint:eslint": "eslint '{src,test}/*'",
    "lint:prettier": "prettier --list-different '**/*.{js,json}'",
    "release": "np",
    "test": "yarn test:types && yarn test:mocha",
    "test:mocha": "yarn build:cjs && mocha --require @babel/register --require source-map-support/register ./test/index.js",
    "test:types": "tsc --noEmit",
    "watch": "yarn build:cjs --watch"
  }
}

And here's what it could be with simple globs (and better performance):

{
  "scripts": {
    "build": "yarn 'build:*'",
    "build:cjs": "rollup --config ./config/rollup-cjs.js",
    "build:docs": "typedoc ./src/index.ts",
    "build:es": "rollup --config ./config/rollup.js",
    "build:max": "rollup --config ./config/rollup-umd.js",
    "build:min": "rollup --config ./config/rollup-umd-min.js",
    "build:types": "tsc --emitDeclarationOnly --declarationMap",
    "check": "yarn '{lint,test}:*'",
    "clean": "rm -rf ./{lib,umd,node_modules}",
    "fix": "yarn 'fix:*'",
    "fix:prettier": "prettier --write '**/*.{js,json,md}'",
    "lint": "yarn 'lint:*'",
    "lint:eslint": "eslint '{src,test}/*'",
    "lint:prettier": "prettier --list-different '**/*.{js,json}'",
    "prepublish": "yarn build",
    "release": "np",
    "test": "yarn 'test:*'",
    "test:mocha": "yarn build:cjs && mocha --require @babel/register --require source-map-support/register ./test/index.js",
    "test:types": "tsc --noEmit",
    "watch": "yarn build:cjs --watch"
  }
}

I think this would have the benefit of requiring no new package.json entries. And it could be considered a superset of the existing yarn run behavior, which is nice because it doesn't really make it harder to understand.

As for logging, I think that's still to be solved, but it would be the same challenge for either of these.

There is already good prior art for using glob strings like this. Popular CLIs like eslint, prettier, etc. accept quoted glob strings.

@benmvp
Copy link

benmvp commented May 17, 2024

@kaylie-alexa can you close this? 😂 it keeps showing up in my TODOs list 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants