-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Support task globs #1088
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
Support task globs #1088
Conversation
uses doublestar, includes tests
supports more doublestar match features, includes test case
@chelkyl is attempting to deploy a commit to the Vercel Team on Vercel. A member of the Team first needs to authorize it. |
Any update to this PR. That would be a great feature. |
^bump, this would be a nice feature! |
Apologies for the delay. I think this is something we'd like to support. I'll get a more in-depth review shortly, but in general, supporting globbing of tasks specified via the commandline sounds like a good feature. |
@@ -75,6 +75,18 @@ Run NPM scripts across all packages in specified scope. Tasks must be specified | |||
to the tasks to be executed. Note that these additional arguments will _not_ be passed to | |||
any additional tasks that are run due to dependencies from the [pipeline](/docs/reference/configuration#pipeline) configuration. | |||
|
|||
Uses glob patterns via [`doublestar`](https://github.com/bmatcuk/doublestar) under the hood. Specifying multiple glob patterns adds to the result. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't call out doublestar
specifically here, since we are not necessarily always going to be using that library. The feature list below is a better solution, since it describes what we will support in the event that we do change our globbing library
|
||
Just a quick overview. | ||
|
||
- `*` matches any number of characters, but not `:` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would this look like if we were not special-casing :
? I'm a little wary of introducing two kinds of globs in the UX.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm opposed to including this at least until we've migrated to Rust. I'm also not convinced of the utility for complex use-cases (but can see the ergonomics win for trivial use-cases).
A few notes:
- Usage of this is going to require quoting the task name in the shell.
~/repos/tests$ turbo eslint-*
ERROR run failed: task `eslint-config-custom` not found in `scripts` in "package.json". Are you sure you added it?
Turbo error: task `eslint-config-custom` not found in `scripts` in "package.json". Are you sure you added it?
~/repos/tests$ ls -1 | grep eslint
eslint-config-custom
~/repos/test$ turbo foo*
zsh: no matches found: foo*
- Task names right now are "just strings" and this creates a new microsyntax for them.
- The special-casing of
:
is a non-starter for me. I get that*
needs an implicit terminator to avoid arbitrary substring matching, but that's asking for trouble.turbo build:*:thing
can matchbuild:one:two:thing
and that's okay. As a user you control task names and you can pattern to avoid this. We could also choose to enforce that task name glob matches/.*\*$/
to make it a suffix match. - Enabling the full globbing power of doublestar feels like far too much rope. We should reduce to just
*
(which would be equivalent to/.*/
. If you need alternation to specify your task names something is wrong. - I generally feel like people just bundle their definition for this inside of
package.json
. People feel like this will get use, but you're likely over-specifying targets. Any non-leaf nodes specified by this are unnecessary. - You can always add a new single leaf node with each of the would-be-glob-targeted tasks.
This:
# Quotes required, don't forget!
turbo "lint:*"
Becomes:
{
"pipeline": {
"lint": {
"dependsOn": ["lint:eslint", "lint:prettier"]
}
}
}
turbo lint
- Paired with
--filter
there's no particular reason to namespace your tasks by package. Packages can already be targeted by glob using path:
turbo build --filter="{packages/*}"
- Not adding this into
dependsOn
creates a distinction in how you address tasks in config and how you address tasks on CLI. That incongruity is unfortunate.
Given all of this I suspect that the value we're targeting here is semi-related tasks.
As we're finishing up our port to Rust, it looks like this code would end up stale-on-arrival! I'm going to close this one up since it hasn't seen too much activity in awhile. |
Uses the already-in-use doublestar library to enable task globbing, supporting most glob features.
Given the following tasks:
This PR adds support for the following:
Closes: #1029