Skip to content

Commit

Permalink
fix: pnpm alias workspace deps (#5569)
Browse files Browse the repository at this point in the history
### Description

Fixes #5441 

Adds support for [referencing workspaces through
aliases](https://pnpm.io/workspaces#referencing-workspace-packages-through-aliases)
by properly resolving them to the correct workspace. Before we would
mark a package as being an external dependency (or if the alias was a
valid workspace depend on the incorrect one).

This PR now recognizes when `workspace:` dependency references a
different package than the name that's used in the `package.json`.

Note for reviewers:
This probably isn't the cleanest solution in either Rust or Go, but
while we need to maintain two codepaths this keeps the code roughly
equivalent.

### Testing Instructions

Added unit tests on the Go side.

Tested manually with a repository where `web` specified it's dependency
on `@scope/ui` as `"ui": "workspace:@scope/ui@*" and verified that:
- `turbo run build`: `@scope/ui` finished building before building
`web`, this hits the Go impl
- `turbo prune --scope=web`: `@scope/ui` was included in the pruned
repository, this hits the Rust impl

---------

Co-authored-by: Chris Olszewski <Chris Olszewski>
  • Loading branch information
chris-olszewski authored Jul 20, 2023
1 parent 396bf45 commit e5f43a5
Show file tree
Hide file tree
Showing 3 changed files with 180 additions and 46 deletions.
34 changes: 31 additions & 3 deletions cli/internal/context/context.go
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,28 @@ 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) (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
// 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.
Expand All @@ -271,11 +293,17 @@ 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) {
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)
}
Expand Down
59 changes: 55 additions & 4 deletions cli/internal/context/context_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -121,15 +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) {
got := isWorkspaceReference(tt.packageVersion, tt.dependencyVersion, pkgDir, rootpath)
if got != tt.want {
t.Errorf("isWorkspaceReference(%v, %v, %v, %v) got = %v, want %v", tt.packageVersion, tt.dependencyVersion, pkgDir, rootpath, got, tt.want)
splitter := dependencySplitter{
workspaces: map[string]*fs.PackageJSON{"@scope/foo": {Version: tt.packageVersion}},
pkgDir: pkgDir,
rootPath: rootpath,
}
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)
})
}
}
Expand Down
133 changes: 94 additions & 39 deletions crates/turborepo-lib/src/package_graph/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 let Some(workspace) = splitter.is_internal(name, version) {
internal.insert(workspace);
} else {
external.insert(Package {
name: name.clone(),
Expand All @@ -483,6 +474,43 @@ impl Dependencies {
}
}

struct DependencySplitter<'a, 'b, 'c> {
repo_root: &'a AbsoluteSystemPath,
workspace_dir: &'b AbsoluteSystemPath,
workspaces: &'c HashMap<WorkspaceName, WorkspaceInfo>,
}

impl<'a, 'b, 'c> DependencySplitter<'a, 'b, 'c> {
fn is_internal(&self, name: &str, version: &str) -> Option<WorkspaceName> {
// TODO implement borrowing for workspaces to allow for zero copy queries
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
.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,
)
});
match is_internal {
true => Some(workspace_name),
false => None,
}
}
}

struct DependencyVersion<'a> {
protocol: Option<&'a str>,
version: &'a str,
Expand Down Expand Up @@ -586,40 +614,67 @@ 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 {
"/some/repo"
})
.unwrap();
let pkg_dir = root.join_components(&["packages", "libA"]);
let workspaces = {
let mut map = HashMap::new();
map.insert(
WorkspaceName::Other("@scope/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
};

let splitter = DependencySplitter {
repo_root: &root,
workspace_dir: &pkg_dir,
workspaces: &workspaces,
};

assert_eq!(
DependencyVersion::new(range).matches_workspace_package(
package_version,
&pkg_dir,
&root
),
expected
splitter.is_internal(dependency_name.unwrap_or("@scope/foo"), range),
expected.map(WorkspaceName::from)
);
}

Expand Down

0 comments on commit e5f43a5

Please sign in to comment.