Extract ResolvePackages #830
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/vercel/turbo-site/mLNofRE3WeCn3xouzGxU89xEPjxV |
|
Two of the tests I've added currently fail, but I think they should pass. |
cli/internal/scope/scope_test.go
Outdated
| { | ||
| // a non-dependent lib changed | ||
| changed: []string{"libs/libC/src/index.ts"}, | ||
| expected: []string{}, |
There was a problem hiding this comment.
I think this is correct, given that we've supplied a scope which does not depend on the changed package. However our current scope resolution logic includes all changed packages in this situation.
cli/internal/scope/scope_test.go
Outdated
| { | ||
| changed: []string{"libs/libB/src/index.ts"}, | ||
| // expect everything, global changed, no scope | ||
| expected: []string{"app1", "app2", "libA", "libB", "libC"}, |
There was a problem hiding this comment.
I think this is correct because there was a global change. A global change should imply a build of everything in scope.
| includeDependents = true | ||
|
|
||
| } | ||
| tui.Output(fmt.Sprintf(ui.Dim("• Packages changed since %s in scope: %s"), opts.Since, strings.Join(filteredPkgs.UnsafeListOfStrings(), ", "))) |
There was a problem hiding this comment.
in the --plan PR we can abstract this into returning some state variable and printing JSON output.
| @@ -919,6 +809,9 @@ func parseRunArgs(args []string, output cli.Ui) (*RunOptions, error) { | |||
| } | |||
| } | |||
| } | |||
| if len(runOptions.scope) != 0 && runOptions.since != "" && !includDepsSet { | |||
There was a problem hiding this comment.
why not just --scope or --since?
| @@ -829,6 +712,11 @@ func parseRunArgs(args []string, output cli.Ui) (*RunOptions, error) { | |||
|
|
|||
| unresolvedCacheFolder := filepath.FromSlash("./node_modules/.cache/turbo") | |||
|
|
|||
| // --scope and --since implies --include-dependencies for backwards compatibility | |||
There was a problem hiding this comment.
should it be --scope or --since?
There was a problem hiding this comment.
--since-only was previously handled here: https://github.com/vercel/turborepo/blob/main/cli/internal/run/run.go#L250
It does not pull in dependencies.
--scope-only was previously handled here: https://github.com/vercel/turborepo/blob/main/cli/internal/run/run.go#L253
It also does not pull in dependencies.
It was only when they were both specified, and there were actual changed packages, that dependencies were included: https://github.com/vercel/turborepo/blob/main/cli/internal/run/run.go#L238
This amounts to building all changed packages and their dependencies, regardless of specified scope. I don't think this behavior is correct, but I'm not sure if we want to stop building dependencies in that case since users might be relying on it? Although they are somewhat arbitrary...
There was a problem hiding this comment.
Right, what I'm saying is that I think in addition to these changes, passing either --scope or --since should automatically --include-dependencies and --no-deps flags (breaking behavior).
There was a problem hiding this comment.
Oh, gotcha. Yeah, we can do that.
There was a problem hiding this comment.
As mentioned here, we're going to keep the behavior in this PR, without expanding it to "--scope or --since triggers --include-dependencies. This closely mirrors existing behavior. We can then look at alternate ways of specifying packages.
scopepackageSCMinterface to make it easier to mock in testsReview Note:
scope_test.goprovides examples of parsed flags and how they translate to packages considered in-scope. I've done my best to infer what packages should be built in each scenario, but please verify. Also very happy to accept additions to that test.Possibly controversial change:
To maintain backwards compatibility, specifying
--sinceand--scopetoturbo runwill imply--include-dependencies. This means that using scope will generally cause all dependencies of the specified scopes to be built, although the cache will still be used so this may potentially be cheap.Previously, this behavior was triggered when
--scopeand--sinceconstraints produced packages to be built. In cases where they were both specified but one or both did not produce packages to be built, dependencies were not included.With this change, the behavior will be consistent based on commandline flags and default to building dependencies. With an upcoming switch over to
cobrafor commandline parsing, we should be able to flag off building dependencies if the user requests it.