From 4070228f09cdbd8697c0d512a8dc185b8cae6ff9 Mon Sep 17 00:00:00 2001 From: Chris Olszewski Date: Wed, 19 Jul 2023 14:57:13 -0700 Subject: [PATCH 1/2] prefactor: add dependency splitter helper --- cli/internal/context/context.go | 19 ++++- cli/internal/context/context_test.go | 7 +- .../src/package_graph/builder.rs | 79 +++++++++++++------ 3 files changed, 79 insertions(+), 26 deletions(-) diff --git a/cli/internal/context/context.go b/cli/internal/context/context.go index 02dac233d618e..98b30c40d2612 100644 --- a/cli/internal/context/context.go +++ b/cli/internal/context/context.go @@ -247,6 +247,17 @@ func (c *Context) resolveWorkspaceRootDeps(rootPackageJSON *fs.PackageJSON, warn return nil } +type dependencySplitter struct { + workspaces map[string]*fs.PackageJSON + pkgDir string + rootPath string +} + +func (d *dependencySplitter) isInternal(name, version string) bool { + item, ok := d.workspaces[name] + return ok && isWorkspaceReference(item.Version, version, d.pkgDir, d.rootPath) +} + // populateWorkspaceGraphForPackageJSON fills in the edges for the dependencies of the given package // that are within the monorepo, as well as collecting and hashing the dependencies of the package // that are not within the monorepo. The vertexName is used to override the package name in the graph. @@ -271,9 +282,15 @@ func (c *Context) populateWorkspaceGraphForPackageJSON(pkg *fs.PackageJSON, root depMap[dep] = version } + splitter := dependencySplitter{ + workspaces: c.WorkspaceInfos.PackageJSONs, + pkgDir: pkg.Dir.ToStringDuringMigration(), + rootPath: rootpath, + } + // split out internal vs. external deps for depName, depVersion := range depMap { - if item, ok := c.WorkspaceInfos.PackageJSONs[depName]; ok && isWorkspaceReference(item.Version, depVersion, pkg.Dir.ToStringDuringMigration(), rootpath) { + if splitter.isInternal(depName, depVersion) { internalDepsSet.Add(depName) c.WorkspaceGraph.Connect(dag.BasicEdge(vertexName, depName)) } else { diff --git a/cli/internal/context/context_test.go b/cli/internal/context/context_test.go index ce67f12c63c1c..a931e53c65e4a 100644 --- a/cli/internal/context/context_test.go +++ b/cli/internal/context/context_test.go @@ -126,7 +126,12 @@ func Test_isWorkspaceReference(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got := isWorkspaceReference(tt.packageVersion, tt.dependencyVersion, pkgDir, rootpath) + splitter := dependencySplitter{ + workspaces: map[string]*fs.PackageJSON{"foo": {Version: tt.packageVersion}}, + pkgDir: pkgDir, + rootPath: rootpath, + } + got := splitter.isInternal("foo", tt.dependencyVersion) if got != tt.want { t.Errorf("isWorkspaceReference(%v, %v, %v, %v) got = %v, want %v", tt.packageVersion, tt.dependencyVersion, pkgDir, rootpath, got, tt.want) } diff --git a/crates/turborepo-lib/src/package_graph/builder.rs b/crates/turborepo-lib/src/package_graph/builder.rs index fadf5d26f1dfe..03cca4e6de78a 100644 --- a/crates/turborepo-lib/src/package_graph/builder.rs +++ b/crates/turborepo-lib/src/package_graph/builder.rs @@ -455,23 +455,14 @@ impl Dependencies { .expect("package.json path should have parent"); let mut internal = HashSet::new(); let mut external = HashSet::new(); + let splitter = DependencySplitter { + repo_root, + workspace_dir, + workspaces, + }; for (name, version) in dependencies.into_iter() { - // TODO implement borrowing for workspaces to allow for zero copy queries - let workspace_name = WorkspaceName::Other(name.clone()); - let is_internal = workspaces - .get(&workspace_name) - // This is the current Go behavior, in the future we might not want to paper over a - // missing version - .map(|e| e.package_json.version.as_deref().unwrap_or_default()) - .map_or(false, |workspace_version| { - DependencyVersion::new(version).matches_workspace_package( - workspace_version, - workspace_dir, - repo_root, - ) - }); - if is_internal { - internal.insert(workspace_name); + if splitter.is_internal(name, version) { + internal.insert(WorkspaceName::Other(name.clone())); } else { external.insert(Package { name: name.clone(), @@ -483,6 +474,31 @@ impl Dependencies { } } +struct DependencySplitter<'a, 'b, 'c> { + repo_root: &'a AbsoluteSystemPath, + workspace_dir: &'b AbsoluteSystemPath, + workspaces: &'c HashMap, +} + +impl<'a, 'b, 'c> DependencySplitter<'a, 'b, 'c> { + fn is_internal(&self, name: &str, version: &str) -> bool { + // TODO implement borrowing for workspaces to allow for zero copy queries + let workspace_name = WorkspaceName::Other(name.to_string()); + self.workspaces + .get(&workspace_name) + // This is the current Go behavior, in the future we might not want to paper over a + // missing version + .map(|e| e.package_json.version.as_deref().unwrap_or_default()) + .map_or(false, |workspace_version| { + DependencyVersion::new(version).matches_workspace_package( + workspace_version, + self.workspace_dir, + self.repo_root, + ) + }) + } +} + struct DependencyVersion<'a> { protocol: Option<&'a str>, version: &'a str, @@ -612,15 +628,30 @@ mod test { }) .unwrap(); let pkg_dir = root.join_components(&["packages", "libA"]); + let workspaces = { + let mut map = HashMap::new(); + map.insert( + WorkspaceName::Other("foo".to_string()), + WorkspaceInfo { + package_json: PackageJson { + version: Some(package_version.to_string()), + ..Default::default() + }, + package_json_path: AnchoredSystemPathBuf::from_raw("unused").unwrap(), + unresolved_external_dependencies: None, + transitive_dependencies: None, + }, + ); + map + }; - assert_eq!( - DependencyVersion::new(range).matches_workspace_package( - package_version, - &pkg_dir, - &root - ), - expected - ); + let splitter = DependencySplitter { + repo_root: &root, + workspace_dir: &pkg_dir, + workspaces: &workspaces, + }; + + assert_eq!(splitter.is_internal("foo", range), expected); } #[test] From 21fec8847255124fa981ee6fc81cf817ffe2fc87 Mon Sep 17 00:00:00 2001 From: Chris Olszewski Date: Wed, 19 Jul 2023 16:28:36 -0700 Subject: [PATCH 2/2] add support for workspace import aliases --- cli/internal/context/context.go | 23 ++++-- cli/internal/context/context_test.go | 56 ++++++++++++-- .../src/package_graph/builder.rs | 74 ++++++++++++------- 3 files changed, 117 insertions(+), 36 deletions(-) diff --git a/cli/internal/context/context.go b/cli/internal/context/context.go index 98b30c40d2612..9c100c10fc0c6 100644 --- a/cli/internal/context/context.go +++ b/cli/internal/context/context.go @@ -253,9 +253,20 @@ type dependencySplitter struct { rootPath string } -func (d *dependencySplitter) isInternal(name, version string) bool { - item, ok := d.workspaces[name] - return ok && isWorkspaceReference(item.Version, version, d.pkgDir, d.rootPath) +func (d *dependencySplitter) isInternal(name, version string) (string, bool) { + resolvedName := name + if withoutProtocol := strings.TrimPrefix(version, "workspace:"); withoutProtocol != version { + parts := strings.Split(withoutProtocol, "@") + lastIndex := len(parts) - 1 + if len(parts) > 1 && (parts[lastIndex] == "*" || parts[lastIndex] == "^" || parts[lastIndex] == "~") { + resolvedName = strings.Join(parts[:lastIndex], "@") + } + } + item, ok := d.workspaces[resolvedName] + if ok && isWorkspaceReference(item.Version, version, d.pkgDir, d.rootPath) { + return resolvedName, true + } + return "", false } // populateWorkspaceGraphForPackageJSON fills in the edges for the dependencies of the given package @@ -290,9 +301,9 @@ func (c *Context) populateWorkspaceGraphForPackageJSON(pkg *fs.PackageJSON, root // split out internal vs. external deps for depName, depVersion := range depMap { - if splitter.isInternal(depName, depVersion) { - internalDepsSet.Add(depName) - c.WorkspaceGraph.Connect(dag.BasicEdge(vertexName, depName)) + if name, internal := splitter.isInternal(depName, depVersion); internal { + internalDepsSet.Add(name) + c.WorkspaceGraph.Connect(dag.BasicEdge(vertexName, name)) } else { externalUnresolvedDepsSet.Add(depName) } diff --git a/cli/internal/context/context_test.go b/cli/internal/context/context_test.go index a931e53c65e4a..d6191fe86ec3a 100644 --- a/cli/internal/context/context_test.go +++ b/cli/internal/context/context_test.go @@ -17,7 +17,7 @@ import ( "gotest.tools/v3/assert" ) -func Test_isWorkspaceReference(t *testing.T) { +func Test_isInternal(t *testing.T) { rootpath, err := filepath.Abs(filepath.FromSlash("/some/repo")) if err != nil { t.Fatalf("failed to create absolute root path %v", err) @@ -30,19 +30,23 @@ func Test_isWorkspaceReference(t *testing.T) { name string packageVersion string dependencyVersion string + depName string want bool + wantDepName string }{ { name: "handles exact match", packageVersion: "1.2.3", dependencyVersion: "1.2.3", want: true, + wantDepName: "@scope/foo", }, { name: "handles semver range satisfied", packageVersion: "1.2.3", dependencyVersion: "^1.0.0", want: true, + wantDepName: "@scope/foo", }, { name: "handles semver range not-satisfied", @@ -55,18 +59,28 @@ func Test_isWorkspaceReference(t *testing.T) { packageVersion: "1.2.3", dependencyVersion: "workspace:1.2.3", want: true, + wantDepName: "@scope/foo", }, { name: "handles workspace protocol with relative path", packageVersion: "1.2.3", dependencyVersion: "workspace:../other-package/", want: true, + wantDepName: "@scope/foo", + }, + { + name: "handles workspace protocol with relative path", + packageVersion: "1.2.3", + dependencyVersion: "workspace:../@scope/foo", + want: true, + wantDepName: "@scope/foo", }, { 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. + wantDepName: "@scope/foo", }, { name: "handles npm protocol with non-satisfied semver range", @@ -85,18 +99,21 @@ func Test_isWorkspaceReference(t *testing.T) { packageVersion: "sometag", dependencyVersion: "1.2.3", want: true, // for backwards compatability with the code before versions were verified + wantDepName: "@scope/foo", }, { name: "handles non-semver package version", packageVersion: "1.2.3", dependencyVersion: "sometag", want: true, // for backwards compatability with the code before versions were verified + wantDepName: "@scope/foo", }, { name: "handles file:... inside repo", packageVersion: "1.2.3", dependencyVersion: "file:../libB", want: true, // this is a sibling package + wantDepName: "@scope/foo", }, { name: "handles file:... outside repo", @@ -109,6 +126,7 @@ func Test_isWorkspaceReference(t *testing.T) { packageVersion: "1.2.3", dependencyVersion: "link:../libB", want: true, // this is a sibling package + wantDepName: "@scope/foo", }, { name: "handles link:... outside repo", @@ -121,20 +139,48 @@ func Test_isWorkspaceReference(t *testing.T) { packageVersion: "0.0.0-development", dependencyVersion: "*", want: true, // "*" should always match + wantDepName: "@scope/foo", + }, + { + name: "handles pnpm alias star", + packageVersion: "1.2.3", + depName: "foo", + dependencyVersion: "workspace:@scope/foo@*", + want: true, + wantDepName: "@scope/foo", + }, + { + name: "handles pnpm alias tilda", + packageVersion: "1.2.3", + depName: "foo", + dependencyVersion: "workspace:@scope/foo@~", + want: true, + wantDepName: "@scope/foo", + }, + { + name: "handles pnpm alias caret", + packageVersion: "1.2.3", + depName: "foo", + dependencyVersion: "workspace:@scope/foo@^", + want: true, + wantDepName: "@scope/foo", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { splitter := dependencySplitter{ - workspaces: map[string]*fs.PackageJSON{"foo": {Version: tt.packageVersion}}, + workspaces: map[string]*fs.PackageJSON{"@scope/foo": {Version: tt.packageVersion}}, pkgDir: pkgDir, rootPath: rootpath, } - got := splitter.isInternal("foo", tt.dependencyVersion) - if got != tt.want { - t.Errorf("isWorkspaceReference(%v, %v, %v, %v) got = %v, want %v", tt.packageVersion, tt.dependencyVersion, pkgDir, rootpath, got, tt.want) + depName := tt.depName + if depName == "" { + depName = "@scope/foo" } + name, got := splitter.isInternal(depName, tt.dependencyVersion) + assert.Equal(t, got, tt.want, tt.name) + assert.Equal(t, name, tt.wantDepName, tt.name) }) } } diff --git a/crates/turborepo-lib/src/package_graph/builder.rs b/crates/turborepo-lib/src/package_graph/builder.rs index 03cca4e6de78a..57fe3d769e37f 100644 --- a/crates/turborepo-lib/src/package_graph/builder.rs +++ b/crates/turborepo-lib/src/package_graph/builder.rs @@ -461,8 +461,8 @@ impl Dependencies { workspaces, }; for (name, version) in dependencies.into_iter() { - if splitter.is_internal(name, version) { - internal.insert(WorkspaceName::Other(name.clone())); + if let Some(workspace) = splitter.is_internal(name, version) { + internal.insert(workspace); } else { external.insert(Package { name: name.clone(), @@ -481,10 +481,18 @@ struct DependencySplitter<'a, 'b, 'c> { } impl<'a, 'b, 'c> DependencySplitter<'a, 'b, 'c> { - fn is_internal(&self, name: &str, version: &str) -> bool { + fn is_internal(&self, name: &str, version: &str) -> Option { // TODO implement borrowing for workspaces to allow for zero copy queries - let workspace_name = WorkspaceName::Other(name.to_string()); - self.workspaces + let workspace_name = WorkspaceName::Other( + version + .strip_prefix("workspace:") + .and_then(|version| version.rsplit_once('@')) + .filter(|(_, version)| *version == "*" || *version == "^" || *version == "~") + .map_or(name, |(actual_name, _)| actual_name) + .to_string(), + ); + let is_internal = self + .workspaces .get(&workspace_name) // This is the current Go behavior, in the future we might not want to paper over a // missing version @@ -495,7 +503,11 @@ impl<'a, 'b, 'c> DependencySplitter<'a, 'b, 'c> { self.workspace_dir, self.repo_root, ) - }) + }); + match is_internal { + true => Some(workspace_name), + false => None, + } } } @@ -602,25 +614,34 @@ mod test { use super::*; - #[test_case("1.2.3", "1.2.3", true ; "handles exact match")] - #[test_case("1.2.3", "^1.0.0", true ; "handles semver range satisfied")] - #[test_case("2.3.4", "^1.0.0", false ; "handles semver range not satisfied")] - #[test_case("1.2.3", "workspace:1.2.3", true ; "handles workspace protocol with version")] - #[test_case("1.2.3", "workspace:*", true ; "handles workspace protocol with no version")] - #[test_case("1.2.3", "workspace:../other-packages/", true ; "handles workspace protocol with relative path")] - #[test_case("1.2.3", "npm:^1.2.3", true ; "handles npm protocol with satisfied semver range")] - #[test_case("2.3.4", "npm:^1.2.3", false ; "handles npm protocol with not satisfied semver range")] - #[test_case("1.2.3", "1.2.2-alpha-123abcd.0", false ; "handles pre-release versions")] + #[test_case("1.2.3", None, "1.2.3", Some("@scope/foo") ; "handles exact match")] + #[test_case("1.2.3", None, "^1.0.0", Some("@scope/foo") ; "handles semver range satisfied")] + #[test_case("2.3.4", None, "^1.0.0", None ; "handles semver range not satisfied")] + #[test_case("1.2.3", None, "workspace:1.2.3", Some("@scope/foo") ; "handles workspace protocol with version")] + #[test_case("1.2.3", None, "workspace:*", Some("@scope/foo") ; "handles workspace protocol with no version")] + #[test_case("1.2.3", None, "workspace:../other-packages/", Some("@scope/foo") ; "handles workspace protocol with relative path")] + #[test_case("1.2.3", None, "workspace:../@scope/foo", Some("@scope/foo") ; "handles workspace protocol with scoped relative path")] + #[test_case("1.2.3", None, "npm:^1.2.3", Some("@scope/foo") ; "handles npm protocol with satisfied semver range")] + #[test_case("2.3.4", None, "npm:^1.2.3", None ; "handles npm protocol with not satisfied semver range")] + #[test_case("1.2.3", None, "1.2.2-alpha-123abcd.0", None ; "handles pre-release versions")] // for backwards compatability with the code before versions were verified - #[test_case("sometag", "1.2.3", true ; "handles non-semver package version")] + #[test_case("sometag", None, "1.2.3", Some("@scope/foo") ; "handles non-semver package version")] // for backwards compatability with the code before versions were verified - #[test_case("1.2.3", "sometag", true ; "handles non-semver dependency version")] - #[test_case("1.2.3", "file:../libB", true ; "handles file:.. inside repo")] - #[test_case("1.2.3", "file:../../../otherproject", false ; "handles file:.. outside repo")] - #[test_case("1.2.3", "link:../libB", true ; "handles link:.. inside repo")] - #[test_case("1.2.3", "link:../../../otherproject", false ; "handles link:.. outside repo")] - #[test_case("0.0.0-development", "*", true ; "handles development versions")] - fn test_matches_workspace_package(package_version: &str, range: &str, expected: bool) { + #[test_case("1.2.3", None, "sometag", Some("@scope/foo") ; "handles non-semver dependency version")] + #[test_case("1.2.3", None, "file:../libB", Some("@scope/foo") ; "handles file:.. inside repo")] + #[test_case("1.2.3", None, "file:../../../otherproject", None ; "handles file:.. outside repo")] + #[test_case("1.2.3", None, "link:../libB", Some("@scope/foo") ; "handles link:.. inside repo")] + #[test_case("1.2.3", None, "link:../../../otherproject", None ; "handles link:.. outside repo")] + #[test_case("0.0.0-development", None, "*", Some("@scope/foo") ; "handles development versions")] + #[test_case("1.2.3", Some("foo"), "workspace:@scope/foo@*", Some("@scope/foo") ; "handles pnpm alias star")] + #[test_case("1.2.3", Some("foo"), "workspace:@scope/foo@~", Some("@scope/foo") ; "handles pnpm alias tilda")] + #[test_case("1.2.3", Some("foo"), "workspace:@scope/foo@^", Some("@scope/foo") ; "handles pnpm alias caret")] + fn test_matches_workspace_package( + package_version: &str, + dependency_name: Option<&str>, + range: &str, + expected: Option<&str>, + ) { let root = AbsoluteSystemPathBuf::new(if cfg!(windows) { "C:\\some\\repo" } else { @@ -631,7 +652,7 @@ mod test { let workspaces = { let mut map = HashMap::new(); map.insert( - WorkspaceName::Other("foo".to_string()), + WorkspaceName::Other("@scope/foo".to_string()), WorkspaceInfo { package_json: PackageJson { version: Some(package_version.to_string()), @@ -651,7 +672,10 @@ mod test { workspaces: &workspaces, }; - assert_eq!(splitter.is_internal("foo", range), expected); + assert_eq!( + splitter.is_internal(dependency_name.unwrap_or("@scope/foo"), range), + expected.map(WorkspaceName::from) + ); } #[test]