From d176f03444251de69f77d7dd2a05052410a65a3c Mon Sep 17 00:00:00 2001 From: "Adam J. Hines" Date: Tue, 1 Mar 2022 11:03:00 -0700 Subject: [PATCH] fix(dep-graph): allow mix of internal and external deps with same name When turbo currently tries to build the dependency graph, it doesn't consider the specified version/range of the dependency, it just assumes that if that package name exists in the workspace it must be an internal dependency. Because of this the cycles are detected and the process hangs. This PR adds some additional logic to ensure that dependencies are only considered internal if the local/workspace package's version matches the semver range specified for the dependency. Fixes: #796 --- cli/internal/context/context.go | 83 +++++++++++++++++++++++----- cli/internal/context/context_test.go | 79 ++++++++++++++++++++++++++ 2 files changed, 148 insertions(+), 14 deletions(-) diff --git a/cli/internal/context/context.go b/cli/internal/context/context.go index cc29d3349e234..92928f71d52ef 100644 --- a/cli/internal/context/context.go +++ b/cli/internal/context/context.go @@ -18,6 +18,7 @@ import ( "github.com/vercel/turborepo/cli/internal/globby" "github.com/vercel/turborepo/cli/internal/util" + "github.com/Masterminds/semver" mapset "github.com/deckarep/golang-set" "github.com/pyr-sh/dag" gitignore "github.com/sabhiram/go-gitignore" @@ -57,6 +58,57 @@ func New(opts ...Option) (*Context, error) { return &m, nil } +// Splits "npm:^1.2.3" and "github:foo/bar.git" into a protocol part and a version part. +func parseDependencyProtocol(version string) (string, string) { + parts := strings.Split(version, ":") + if len(parts) == 1 { + return "", parts[0] + } + + return parts[0], strings.Join(parts[1:], ":") +} + +func isProtocolExternal(protocol string) bool { + // The npm protocol for yarn by default still uses the workspace package if the workspace + // version is in a compatible semver range. See https://github.com/yarnpkg/berry/discussions/4015 + // For now, we will just assume if the npm protocol is being used and the version matches + // its an internal dependency which matches the existing behavior before this additional + // logic was added. + + // TODO: extend this to support the `enableTransparentWorkspaces` yarn option + return protocol != "" && protocol != "npm" +} + +func isWorkspaceReference(packageVersion string, dependencyVersion string) bool { + protocol, dependencyVersion := parseDependencyProtocol(dependencyVersion) + + if protocol == "workspace" { + // TODO: Since support at the moment is non-existent for workspaces that contain multiple + // versions of the same package name, just assume its a match and don't check the range + // for an exact match. + return true + } else if isProtocolExternal(protocol) { + // Other protocols are assumed to be external references ("github:", "link:", "file:" etc) + return false + } + + // If we got this far, then we need to check the workspace package version to see it satisfies + // the dependencies range to determin whether or not its an internal or external dependency. + + constraint, constraintErr := semver.NewConstraint(dependencyVersion) + pkgVersion, packageVersionErr := semver.NewVersion(packageVersion) + if constraintErr != nil || packageVersionErr != nil { + // For backwards compatibility with existing behavior, if we can't parse the version then we + // treat the dependency as an internal package reference and swallow the error. + + // TODO: some package managers also support tags like "latest". Does extra handling need to be + // added for this corner-case + return true + } + + return constraint.Check(pkgVersion) +} + func WithGraph(rootpath string, config *config.Config) Option { return func(c *Context) error { c.PackageInfos = make(map[interface{}]*fs.PackageJSON) @@ -283,35 +335,38 @@ func GetTargetsFromArguments(arguments []string, configJson *fs.TurboConfigJSON) func (c *Context) populateTopologicGraphForPackageJson(pkg *fs.PackageJSON) error { c.mutex.Lock() defer c.mutex.Unlock() + depMap := make(map[string]string) internalDepsSet := make(dag.Set) - depSet := make(dag.Set) + externalUnresolvedDepsSet := make(dag.Set) externalDepSet := mapset.NewSet() pkg.UnresolvedExternalDeps = make(map[string]string) - for dep := range pkg.Dependencies { - depSet.Add(dep) + + for dep, version := range pkg.Dependencies { + depMap[dep] = version } - for dep := range pkg.DevDependencies { - depSet.Add(dep) + for dep, version := range pkg.DevDependencies { + depMap[dep] = version } - for dep := range pkg.OptionalDependencies { - depSet.Add(dep) + for dep, version := range pkg.OptionalDependencies { + depMap[dep] = version } - for dep := range pkg.PeerDependencies { - depSet.Add(dep) + for dep, version := range pkg.PeerDependencies { + depMap[dep] = version } // split out internal vs. external deps - for _, dependencyName := range depSet.List() { - if item, ok := c.PackageInfos[dependencyName]; ok { - internalDepsSet.Add(item.Name) - c.TopologicalGraph.Connect(dag.BasicEdge(pkg.Name, dependencyName)) + for depName, depVersion := range depMap { + if item, ok := c.PackageInfos[depName]; ok && isWorkspaceReference(item.Version, depVersion) { + internalDepsSet.Add(depName) + c.TopologicalGraph.Connect(dag.BasicEdge(pkg.Name, depName)) + } else { + externalUnresolvedDepsSet.Add(depName) } } - externalUnresolvedDepsSet := depSet.Difference(internalDepsSet) for _, name := range externalUnresolvedDepsSet.List() { name := name.(string) if item, ok := pkg.Dependencies[name]; ok { diff --git a/cli/internal/context/context_test.go b/cli/internal/context/context_test.go index 315361b8a53c3..303ffe4b392ff 100644 --- a/cli/internal/context/context_test.go +++ b/cli/internal/context/context_test.go @@ -114,3 +114,82 @@ func Test_getHashableTurboEnvVarsFromOs(t *testing.T) { t.Errorf("getHashableTurboEnvVarsFromOs() env pairs got = %v, want %v", gotPairs, wantPairs) } } + +func Test_isWorkspaceReference(t *testing.T) { + tests := []struct { + name string + packageVersion string + dependencyVersion string + want bool + }{ + { + name: "handles exact match", + packageVersion: "1.2.3", + dependencyVersion: "1.2.3", + want: true, + }, + { + name: "handles semver range satisfied", + packageVersion: "1.2.3", + dependencyVersion: "^1.0.0", + want: true, + }, + { + name: "handles semver range not-satisfied", + packageVersion: "2.3.4", + dependencyVersion: "^1.0.0", + want: false, + }, + { + name: "handles workspace protocol with version", + packageVersion: "1.2.3", + dependencyVersion: "workspace:1.2.3", + want: true, + }, + { + name: "handles workspace protocol with relative path", + packageVersion: "1.2.3", + dependencyVersion: "workspace:../other-package/", + want: true, + }, + { + name: "handles npm protocol with satisfied semver range", + packageVersion: "1.2.3", + dependencyVersion: "npm:^1.2.3", + want: true, // default in yarn is to use the workspace version unless `enableTransparentWorkspaces: true`. This isn't currently being checked. + }, + { + name: "handles npm protocol with non-satisfied semver range", + packageVersion: "2.3.4", + dependencyVersion: "npm:^1.2.3", + want: false, + }, + { + name: "handles pre-release versions", + packageVersion: "1.2.3", + dependencyVersion: "1.2.2-alpha-1234abcd.0", + want: false, + }, + { + name: "handles non-semver package version", + packageVersion: "sometag", + dependencyVersion: "1.2.3", + want: true, // for backwards compatability with the code before versions were verified + }, + { + name: "handles non-semver package version", + packageVersion: "1.2.3", + dependencyVersion: "sometag", + want: true, // for backwards compatability with the code before versions were verified + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := isWorkspaceReference(tt.packageVersion, tt.dependencyVersion) + if got != tt.want { + t.Errorf("isWorkspaceReference() got = %v, want %v", got, tt.want) + } + }) + } +}