Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: support pnpm workspace path aliasing #7512

Merged
merged 8 commits into from
Feb 29, 2024
Merged

fix: support pnpm workspace path aliasing #7512

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2357e5b
chore: move dependency splitter to own module
chris-olszewski Feb 22, 2024
8cb5c20
chore: simplify lifetimes in splitter
chris-olszewski Feb 22, 2024
11bad04
chore: expand test fixture
chris-olszewski Feb 22, 2024
78d5c16
chore: prefactor out workspace protocol parsing
chris-olszewski Feb 22, 2024
0d851d7
chore: move fetching of workspace package into helper
chris-olszewski Feb 22, 2024
526cdb7
chore: add failing test for fix
chris-olszewski Feb 22, 2024
bfc203c
fix(pnpm): correctly select package based on path
chris-olszewski Feb 23, 2024
dc3d6c4
fix merge conflicts from caused by 7506
chris-olszewski Feb 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
201 changes: 5 additions & 196 deletions crates/turborepo-repository/src/package_graph/builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,19 @@
use std::{
backtrace::Backtrace,
collections::{BTreeMap, HashMap, HashSet},
fmt,
};

use petgraph::graph::{Graph, NodeIndex};
use tracing::{warn, Instrument};
use turbopath::{
AbsoluteSystemPath, AbsoluteSystemPathBuf, AnchoredSystemPath, AnchoredSystemPathBuf,
RelativeUnixPathBuf,
};
use turborepo_graph_utils as graph;
use turborepo_lockfiles::Lockfile;

use super::{PackageGraph, PackageInfo, PackageName, PackageNode};
use super::{
dep_splitter::DependencySplitter, PackageGraph, PackageInfo, PackageName, PackageNode,
};
use crate::{
discovery::{
self, CachingPackageDiscovery, LocalPackageDiscoveryBuilder, PackageDiscovery,
Expand Down Expand Up @@ -541,11 +541,7 @@ impl Dependencies {
.expect("package.json path should have parent");
let mut internal = HashSet::new();
let mut external = BTreeMap::new();
let splitter = DependencySplitter {
repo_root,
workspace_dir,
workspaces,
};
let splitter = DependencySplitter::new(repo_root, workspace_dir, workspaces);
for (name, version) in dependencies.into_iter() {
if let Some(workspace) = splitter.is_internal(name, version) {
internal.insert(workspace);
Expand All @@ -557,128 +553,6 @@ impl Dependencies {
}
}

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

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

impl<'a> DependencyVersion<'a> {
fn new(qualified_version: &'a str) -> Self {
qualified_version.split_once(':').map_or(
Self {
protocol: None,
version: qualified_version,
},
|(protocol, version)| Self {
protocol: Some(protocol),
version,
},
)
}

fn is_external(&self) -> 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
self.protocol.map_or(false, |p| p != "npm")
}

fn matches_workspace_package(
&self,
package_version: &str,
cwd: &AbsoluteSystemPath,
root: &AbsoluteSystemPath,
) -> bool {
match self.protocol {
Some("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.
true
}
Some("file") | Some("link") => {
// Default to internal if we have the package but somehow cannot get the path
RelativeUnixPathBuf::new(self.version)
.map(|file_path| cwd.join_unix_path(file_path))
.map_or(true, |dep_path| root.contains(&dep_path))
}
Some(_) if self.is_external() => {
// Other protocols are assumed to be external references ("github:", etc)
false
}
_ if self.version == "*" => true,
_ => {
// 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.
let constraint = node_semver::Range::parse(self.version);
let version = node_semver::Version::parse(package_version);

// 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
constraint
.ok()
.zip(version.ok())
.map_or(true, |(constraint, version)| constraint.satisfies(&version))
}
}
}
}

impl<'a> fmt::Display for DependencyVersion<'a> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self.protocol {
Some(protocol) => f.write_fmt(format_args!("{}:{}", protocol, self.version)),
None => f.write_str(self.version),
}
}
}

impl PackageInfo {
fn unix_dir_str(&self) -> Result<String, Error> {
let unix = self
Expand All @@ -694,75 +568,10 @@ impl PackageInfo {
mod test {
use std::assert_matches::assert_matches;

use test_case::test_case;
use turbopath::AbsoluteSystemPathBuf;

use super::*;

#[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", 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", 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(
PackageName::Other("@scope/foo".to_string()),
PackageInfo {
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!(
splitter.is_internal(dependency_name.unwrap_or("@scope/foo"), range),
expected.map(PackageName::from)
);
}

struct MockDiscovery;
impl PackageDiscovery for MockDiscovery {
async fn discover_packages(
Expand Down Expand Up @@ -811,6 +620,6 @@ mod test {
);
map
}));
assert_matches!(builder.build().await, Err(Error::DuplicateWorkspace { .. }))
assert_matches!(builder.build().await, Err(Error::DuplicateWorkspace { .. }));
}
}
Loading
Loading