Skip to content

Commit

Permalink
Remove dependencies on context methods / functions (vercel#821)
Browse files Browse the repository at this point in the history 
Continuing to remove code dependencies on the `internal/context` package so that we can possibly calculate the values remotely / asynchronously.

 * Move `getTargetsFromArguments` from `context` to `run`, the only place where it is called.
 * `prune` reuses the work done resolving global dependencies during context creation. This both reduces the dependency on a method on `context` as well as avoids doing duplicate work.
 * mark `context` methods private so that we don't accidentally rely on them elsewhere.
  • Loading branch information
Greg Soltis authored Mar 3, 2022
1 parent 854f804 commit f947ece
Show file tree
Hide file tree
Showing 5 changed files with 127 additions and 153 deletions.
42 changes: 9 additions & 33 deletions cli/internal/context/context.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ const GLOBAL_CACHE_KEY = "snozzberries"

// Context of the CLI
type Context struct {
RootPackageInfo *fs.PackageJSON // TODO(gsoltis): should this be included in PackageInfos?
PackageInfos map[interface{}]*fs.PackageJSON
PackageNames []string
TopologicalGraph dag.AcyclicGraph
Expand Down Expand Up @@ -108,9 +109,10 @@ func WithGraph(rootpath string, config *config.Config) Option {
c.Lockfile = lockfile
}

if c.ResolveWorkspaceRootDeps(rootPackageJSON) != nil {
if c.resolveWorkspaceRootDeps(rootPackageJSON) != nil {
return err
}
c.RootPackageInfo = rootPackageJSON

spaces, err := c.Backend.GetWorkspaceGlobs(rootpath)

Expand Down Expand Up @@ -214,7 +216,7 @@ func (c *Context) loadPackageDepsHash(pkg *fs.PackageJSON) error {
return nil
}

func (c *Context) ResolveWorkspaceRootDeps(rootPackageJSON *fs.PackageJSON) error {
func (c *Context) resolveWorkspaceRootDeps(rootPackageJSON *fs.PackageJSON) error {
seen := mapset.NewSet()
var lockfileWg sync.WaitGroup
pkg := rootPackageJSON
Expand All @@ -234,7 +236,7 @@ func (c *Context) ResolveWorkspaceRootDeps(rootPackageJSON *fs.PackageJSON) erro
}
if util.IsYarn(c.Backend.Name) {
pkg.SubLockfile = make(fs.YarnLockfile)
c.ResolveDepGraph(&lockfileWg, pkg.UnresolvedExternalDeps, depSet, seen, pkg)
c.resolveDepGraph(&lockfileWg, pkg.UnresolvedExternalDeps, depSet, seen, pkg)
lockfileWg.Wait()
pkg.ExternalDeps = make([]string, 0, depSet.Cardinality())
for _, v := range depSet.ToSlice() {
Expand All @@ -254,32 +256,6 @@ func (c *Context) ResolveWorkspaceRootDeps(rootPackageJSON *fs.PackageJSON) erro
return nil
}

// GetTargetsFromArguments returns a list of targets from the arguments and Turbo config.
// Return targets are always unique sorted alphabetically.
func GetTargetsFromArguments(arguments []string, configJson *fs.TurboConfigJSON) ([]string, error) {
targets := make(util.Set)
for _, arg := range arguments {
if arg == "--" {
break
}
if !strings.HasPrefix(arg, "-") {
targets.Add(arg)
found := false
for task := range configJson.Pipeline {
if task == arg {
found = true
}
}
if !found {
return nil, fmt.Errorf("task `%v` not found in turbo pipeline in package.json. Are you sure you added it?", arg)
}
}
}
stringTargets := targets.UnsafeListOfStrings()
sort.Strings(stringTargets)
return stringTargets, nil
}

func (c *Context) populateTopologicGraphForPackageJson(pkg *fs.PackageJSON) error {
c.mutex.Lock()
defer c.mutex.Unlock()
Expand Down Expand Up @@ -330,7 +306,7 @@ func (c *Context) populateTopologicGraphForPackageJson(pkg *fs.PackageJSON) erro
pkg.SubLockfile = make(fs.YarnLockfile)
seen := mapset.NewSet()
var lockfileWg sync.WaitGroup
c.ResolveDepGraph(&lockfileWg, pkg.UnresolvedExternalDeps, externalDepSet, seen, pkg)
c.resolveDepGraph(&lockfileWg, pkg.UnresolvedExternalDeps, externalDepSet, seen, pkg)
lockfileWg.Wait()

// when there are no internal dependencies, we need to still add these leafs to the graph
Expand Down Expand Up @@ -376,7 +352,7 @@ func (c *Context) parsePackageJSON(buildFilePath string) error {
return nil
}

func (c *Context) ResolveDepGraph(wg *sync.WaitGroup, unresolvedDirectDeps map[string]string, resolvedDepsSet mapset.Set, seen mapset.Set, pkg *fs.PackageJSON) {
func (c *Context) resolveDepGraph(wg *sync.WaitGroup, unresolvedDirectDeps map[string]string, resolvedDepsSet mapset.Set, seen mapset.Set, pkg *fs.PackageJSON) {
if !util.IsYarn(c.Backend.Name) {
return
}
Expand Down Expand Up @@ -414,10 +390,10 @@ func (c *Context) ResolveDepGraph(wg *sync.WaitGroup, unresolvedDirectDeps map[s
resolvedDepsSet.Add(fmt.Sprintf("%v@%v", directDepName, entry.Version))

if len(entry.Dependencies) > 0 {
c.ResolveDepGraph(wg, entry.Dependencies, resolvedDepsSet, seen, pkg)
c.resolveDepGraph(wg, entry.Dependencies, resolvedDepsSet, seen, pkg)
}
if len(entry.OptionalDependencies) > 0 {
c.ResolveDepGraph(wg, entry.OptionalDependencies, resolvedDepsSet, seen, pkg)
c.resolveDepGraph(wg, entry.OptionalDependencies, resolvedDepsSet, seen, pkg)
}

}(directDepName, unresolvedVersion)
Expand Down
90 changes: 0 additions & 90 deletions cli/internal/context/context_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,98 +3,8 @@ package context
import (
"reflect"
"testing"

"github.com/vercel/turborepo/cli/internal/fs"
)

func TestGetTargetsFromArguments(t *testing.T) {
type args struct {
arguments []string
configJson *fs.TurboConfigJSON
}
tests := []struct {
name string
args args
want []string
wantErr bool
}{
{
name: "handles one defined target",
args: args{
arguments: []string{"build"},
configJson: &fs.TurboConfigJSON{
Pipeline: map[string]fs.Pipeline{
"build": {},
"test": {},
"thing#test": {},
},
},
},
want: []string{"build"},
wantErr: false,
},
{
name: "handles multiple targets and ignores flags",
args: args{
arguments: []string{"build", "test", "--foo", "--bar"},
configJson: &fs.TurboConfigJSON{
Pipeline: map[string]fs.Pipeline{
"build": {},
"test": {},
"thing#test": {},
},
},
},
want: []string{"build", "test"},
wantErr: false,
},
{
name: "handles pass through arguments after -- ",
args: args{
arguments: []string{"build", "test", "--", "--foo", "build", "--cache-dir"},
configJson: &fs.TurboConfigJSON{
Pipeline: map[string]fs.Pipeline{
"build": {},
"test": {},
"thing#test": {},
},
},
},
want: []string{"build", "test"},
wantErr: false,
},
{
name: "handles unknown pipeline targets ",
args: args{
arguments: []string{"foo", "test", "--", "--foo", "build", "--cache-dir"},
configJson: &fs.TurboConfigJSON{
Pipeline: map[string]fs.Pipeline{
"build": {},
"test": {},
"thing#test": {},
},
},
},
want: nil,
wantErr: true,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got, err := GetTargetsFromArguments(tt.args.arguments, tt.args.configJson)
if (err != nil) != tt.wantErr {
t.Errorf("GetTargetsFromArguments() error = %v, wantErr %v", err, tt.wantErr)
return
}

if !reflect.DeepEqual(got, tt.want) {
t.Errorf("GetTargetsFromArguments() = %v, want %v", got, tt.want)
}
})
}
}

func Test_getHashableTurboEnvVarsFromOs(t *testing.T) {
env := []string{
"SOME_ENV_VAR=excluded",
Expand Down
30 changes: 1 addition & 29 deletions cli/internal/prune/prune.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,13 @@ import (
"os"
"path/filepath"
"strings"
"sync"

"github.com/vercel/turborepo/cli/internal/config"
"github.com/vercel/turborepo/cli/internal/context"
"github.com/vercel/turborepo/cli/internal/fs"
"github.com/vercel/turborepo/cli/internal/ui"
"github.com/vercel/turborepo/cli/internal/util"

mapset "github.com/deckarep/golang-set"
"github.com/fatih/color"
"github.com/hashicorp/go-hclog"
"github.com/mitchellh/cli"
Expand Down Expand Up @@ -128,33 +126,7 @@ func (c *PruneCommand) Run(args []string) int {
return 1
}
workspaces := []string{}
seen := mapset.NewSet()
var lockfileWg sync.WaitGroup
pkg, err := fs.ReadPackageJSON(filepath.Join(pruneOptions.cwd, "package.json"))
if err != nil {
c.logError(c.Config.Logger, "", fmt.Errorf("could not read package.json: %w", err))
return 1
}
depSet := mapset.NewSet()
pkg.UnresolvedExternalDeps = make(map[string]string)
for dep, version := range pkg.Dependencies {
pkg.UnresolvedExternalDeps[dep] = version
}
for dep, version := range pkg.DevDependencies {
pkg.UnresolvedExternalDeps[dep] = version
}
for dep, version := range pkg.OptionalDependencies {
pkg.UnresolvedExternalDeps[dep] = version
}
for dep, version := range pkg.PeerDependencies {
pkg.UnresolvedExternalDeps[dep] = version
}

pkg.SubLockfile = make(fs.YarnLockfile)
ctx.ResolveDepGraph(&lockfileWg, pkg.UnresolvedExternalDeps, depSet, seen, pkg)

lockfileWg.Wait()
lockfile := pkg.SubLockfile
lockfile := ctx.RootPackageInfo.SubLockfile
targets := []interface{}{pruneOptions.scope}
internalDeps, err := ctx.TopologicalGraph.Ancestors(pruneOptions.scope)
if err != nil {
Expand Down
28 changes: 27 additions & 1 deletion cli/internal/run/run.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ func (c *RunCommand) Run(args []string) int {
c.logError(c.Config.Logger, "", err)
return 1
}
targets, err := context.GetTargetsFromArguments(args, ctx.TurboConfig)
targets, err := getTargetsFromArguments(args, ctx.TurboConfig)
if err != nil {
c.logError(c.Config.Logger, "", fmt.Errorf("failed to resolve targets: %w", err))
return 1
Expand Down Expand Up @@ -1012,3 +1012,29 @@ func replayLogs(logger hclog.Logger, prefixUi cli.Ui, runOptions *RunOptions, lo
}
logger.Debug("finish replaying logs")
}

// GetTargetsFromArguments returns a list of targets from the arguments and Turbo config.
// Return targets are always unique sorted alphabetically.
func getTargetsFromArguments(arguments []string, configJson *fs.TurboConfigJSON) ([]string, error) {
targets := make(util.Set)
for _, arg := range arguments {
if arg == "--" {
break
}
if !strings.HasPrefix(arg, "-") {
targets.Add(arg)
found := false
for task := range configJson.Pipeline {
if task == arg {
found = true
}
}
if !found {
return nil, fmt.Errorf("task `%v` not found in turbo pipeline in package.json. Are you sure you added it?", arg)
}
}
}
stringTargets := targets.UnsafeListOfStrings()
sort.Strings(stringTargets)
return stringTargets, nil
}
90 changes: 90 additions & 0 deletions cli/internal/run/run_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,12 @@ import (
"fmt"
"os"
"path/filepath"
"reflect"
"testing"

"github.com/mitchellh/cli"
"github.com/vercel/turborepo/cli/internal/context"
"github.com/vercel/turborepo/cli/internal/fs"
"github.com/vercel/turborepo/cli/internal/util"

"github.com/stretchr/testify/assert"
Expand Down Expand Up @@ -222,3 +224,91 @@ func TestScopedPackages(t *testing.T) {
assert.Error(t, err)
})
}

func TestGetTargetsFromArguments(t *testing.T) {
type args struct {
arguments []string
configJson *fs.TurboConfigJSON
}
tests := []struct {
name string
args args
want []string
wantErr bool
}{
{
name: "handles one defined target",
args: args{
arguments: []string{"build"},
configJson: &fs.TurboConfigJSON{
Pipeline: map[string]fs.Pipeline{
"build": {},
"test": {},
"thing#test": {},
},
},
},
want: []string{"build"},
wantErr: false,
},
{
name: "handles multiple targets and ignores flags",
args: args{
arguments: []string{"build", "test", "--foo", "--bar"},
configJson: &fs.TurboConfigJSON{
Pipeline: map[string]fs.Pipeline{
"build": {},
"test": {},
"thing#test": {},
},
},
},
want: []string{"build", "test"},
wantErr: false,
},
{
name: "handles pass through arguments after -- ",
args: args{
arguments: []string{"build", "test", "--", "--foo", "build", "--cache-dir"},
configJson: &fs.TurboConfigJSON{
Pipeline: map[string]fs.Pipeline{
"build": {},
"test": {},
"thing#test": {},
},
},
},
want: []string{"build", "test"},
wantErr: false,
},
{
name: "handles unknown pipeline targets ",
args: args{
arguments: []string{"foo", "test", "--", "--foo", "build", "--cache-dir"},
configJson: &fs.TurboConfigJSON{
Pipeline: map[string]fs.Pipeline{
"build": {},
"test": {},
"thing#test": {},
},
},
},
want: nil,
wantErr: true,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got, err := getTargetsFromArguments(tt.args.arguments, tt.args.configJson)
if (err != nil) != tt.wantErr {
t.Errorf("GetTargetsFromArguments() error = %v, wantErr %v", err, tt.wantErr)
return
}

if !reflect.DeepEqual(got, tt.want) {
t.Errorf("GetTargetsFromArguments() = %v, want %v", got, tt.want)
}
})
}
}

0 comments on commit f947ece

Please sign in to comment.