Skip to content

Commit

Permalink
feat: require packageManager in package.json (#8017)
Browse files Browse the repository at this point in the history
### Description

With 2.0 we will now be requiring a `packageManager` field in
`package.json` as this is a best practice and it helps us behave in a
deterministic manner.

The actual code change is very straightforward as we remove our package
manager inference code and return an error if reading package manager
from `package.json` fails.

Most of the PR is updating tests.

### Testing Instructions

Updated unit tests
  • Loading branch information
chris-olszewski committed May 14, 2024
1 parent d893fde commit f8bea45
Show file tree
Hide file tree
Showing 20 changed files with 99 additions and 419 deletions.
4 changes: 3 additions & 1 deletion crates/turborepo-filewatch/src/hash_watcher.rs
Original file line number Diff line number Diff line change
Expand Up @@ -747,7 +747,9 @@ mod tests {
.unwrap();
repo_root
.join_component("package.json")
.create_with_contents(r#"{"workspaces": ["packages/*"]}"#)
.create_with_contents(
r#"{"workspaces": ["packages/*"], "packageManager": "npm@10.0.0"}"#,
)
.unwrap();
repo_root
.join_component("package-lock.json")
Expand Down
30 changes: 18 additions & 12 deletions crates/turborepo-filewatch/src/package_watcher.rs
Original file line number Diff line number Diff line change
Expand Up @@ -600,7 +600,9 @@ mod test {
// write workspaces to root
repo_root
.join_component("package.json")
.create_with_contents(r#"{"workspaces":["packages/*"]}"#)
.create_with_contents(
r#"{"workspaces":["packages/*"], "packageManager": "npm@10.0.0"}"#,
)
.unwrap();

let watcher = FileSystemWatcher::new_with_default_cookie_dir(&repo_root).unwrap();
Expand Down Expand Up @@ -703,7 +705,9 @@ mod test {
// write workspaces to root
repo_root
.join_component("package.json")
.create_with_contents(r#"{"workspaces":["packages/*", "packages2/*"]}"#)
.create_with_contents(
r#"{"workspaces":["packages/*", "packages2/*"], "packageManager": "npm@10.0.0"}"#,
)
.unwrap();

let watcher = FileSystemWatcher::new_with_default_cookie_dir(&repo_root).unwrap();
Expand Down Expand Up @@ -743,7 +747,9 @@ mod test {
// update workspaces to no longer cover packages2
repo_root
.join_component("package.json")
.create_with_contents(r#"{"workspaces":["packages/*"]}"#)
.create_with_contents(
r#"{"workspaces":["packages/*"], "packageManager": "npm@10.0.0"}"#,
)
.unwrap();

let mut data = package_watcher.discover_packages_blocking().await.unwrap();
Expand Down Expand Up @@ -804,7 +810,7 @@ mod test {
let root_package_json_path = repo_root.join_component("package.json");
// Start with no workspace glob
root_package_json_path
.create_with_contents(r#"{"packageManager": "pnpm@7.0"}"#)
.create_with_contents(r#"{"packageManager": "pnpm@7.0.0"}"#)
.unwrap();
repo_root
.join_component("pnpm-lock.yaml")
Expand Down Expand Up @@ -873,7 +879,7 @@ mod test {
let root_package_json_path = repo_root.join_component("package.json");
// Start with no workspace glob
root_package_json_path
.create_with_contents(r#"{"packageManager": "npm@7.0"}"#)
.create_with_contents(r#"{"packageManager": "npm@7.0.0"}"#)
.unwrap();
repo_root
.join_component("package-lock.json")
Expand All @@ -896,7 +902,7 @@ mod test {
.unwrap_err();

root_package_json_path
.create_with_contents(r#"{"packageManager": "pnpm@7.0", "workspaces": ["foo/*"]}"#)
.create_with_contents(r#"{"packageManager": "npm@7.0.0", "workspaces": ["foo/*"]}"#)
.unwrap();

let resp = package_watcher.discover_packages_blocking().await.unwrap();
Expand All @@ -911,7 +917,7 @@ mod test {

// Create an invalid workspace glob
root_package_json_path
.create_with_contents(r#"{"packageManager": "pnpm@7.0", "workspaces": ["foo/***"]}"#)
.create_with_contents(r#"{"packageManager": "npm@7.0.0", "workspaces": ["foo/***"]}"#)
.unwrap();

// We expect an error due to invalid workspace glob
Expand All @@ -922,7 +928,7 @@ mod test {

// Set it back to valid, ensure we recover
root_package_json_path
.create_with_contents(r#"{"packageManager": "pnpm@7.0", "workspaces": ["foo/*"]}"#)
.create_with_contents(r#"{"packageManager": "npm@7.0.0", "workspaces": ["foo/*"]}"#)
.unwrap();
let resp = package_watcher.discover_packages_blocking().await.unwrap();
assert_eq!(resp.package_manager, PackageManager::Npm);
Expand All @@ -945,7 +951,7 @@ mod test {
let root_package_json_path = repo_root.join_component("package.json");
// Start with no workspace glob
root_package_json_path
.create_with_contents(r#"{"packageManager": "pnpm@7.0"}"#)
.create_with_contents(r#"{"packageManager": "pnpm@7.0.0"}"#)
.unwrap();
let pnpm_lock_file = repo_root.join_component("pnpm-lock.yaml");
pnpm_lock_file.create_with_contents("").unwrap();
Expand All @@ -963,8 +969,8 @@ mod test {
let resp = package_watcher.discover_packages_blocking().await.unwrap();
assert_eq!(resp.package_manager, PackageManager::Pnpm);

pnpm_lock_file.remove_file().unwrap();
// No more lock file, verify we're in an invalid state
workspaces_path.remove_file().unwrap();
// No more workspaces file, verify we're in an invalid state
package_watcher
.discover_packages_blocking()
.await
Expand All @@ -980,7 +986,7 @@ mod test {

// update package.json to complete the transition
root_package_json_path
.create_with_contents(r#"{"packageManager": "npm@7.0", "workspaces": ["foo/*"]}"#)
.create_with_contents(r#"{"packageManager": "npm@7.0.0", "workspaces": ["foo/*"]}"#)
.unwrap();
let resp = package_watcher.discover_packages_blocking().await.unwrap();
assert_eq!(resp.package_manager, PackageManager::Npm);
Expand Down
5 changes: 4 additions & 1 deletion crates/turborepo-repository/src/discovery.rs
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,10 @@ impl PackageDiscoveryBuilder for LocalPackageDiscoveryBuilder {
let package_manager = match self.package_manager {
Some(pm) => pm,
None => {
PackageManager::get_package_manager(&self.repo_root, self.package_json.as_ref())?
let package_json = self.package_json.map(Ok).unwrap_or_else(|| {
PackageJson::load(&self.repo_root.join_component("package.json"))
})?;
PackageManager::get_package_manager(&package_json)?
}
};

Expand Down
11 changes: 7 additions & 4 deletions crates/turborepo-repository/src/inference.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,7 @@ impl RepoState {
.ok()
.map(|package_json| {
// FIXME: We should save this package manager that we detected
let package_manager =
PackageManager::get_package_manager(path, Some(&package_json));
let package_manager = PackageManager::get_package_manager(&package_json);
let workspace_globs = package_manager
.as_ref()
.ok()
Expand Down Expand Up @@ -152,7 +151,9 @@ mod test {
let monorepo_pkg_json = monorepo_root.join_component("package.json");
monorepo_pkg_json.ensure_dir().unwrap();
monorepo_pkg_json
.create_with_contents("{\"workspaces\": [\"packages/*\"]}")
.create_with_contents(
"{\"workspaces\": [\"packages/*\"], \"packageManager\": \"npm@7.0.0\"}",
)
.unwrap();
monorepo_root
.join_component("package-lock.json")
Expand Down Expand Up @@ -188,7 +189,9 @@ mod test {
.unwrap();
standalone_monorepo
.join_component("package.json")
.create_with_contents("{\"workspaces\": [\"packages/*\"]}")
.create_with_contents(
"{\"workspaces\": [\"packages/*\"], \"packageManager\": \"npm@7.0.0\"}",
)
.unwrap();
standalone_monorepo
.join_component("package-lock.json")
Expand Down
62 changes: 0 additions & 62 deletions crates/turborepo-repository/src/package_manager/bun.rs
Original file line number Diff line number Diff line change
@@ -1,63 +1 @@
use turbopath::AbsoluteSystemPath;

use crate::package_manager::{Error, PackageManager};

pub const LOCKFILE: &str = "bun.lockb";

pub struct BunDetector<'a> {
repo_root: &'a AbsoluteSystemPath,
found: bool,
}

impl<'a> BunDetector<'a> {
pub fn new(repo_root: &'a AbsoluteSystemPath) -> Self {
Self {
repo_root,
found: false,
}
}
}

impl<'a> Iterator for BunDetector<'a> {
type Item = Result<PackageManager, Error>;

fn next(&mut self) -> Option<Self::Item> {
if self.found {
return None;
}

self.found = true;
let package_json = self.repo_root.join_component(LOCKFILE);

if package_json.exists() {
Some(Ok(PackageManager::Bun))
} else {
None
}
}
}

#[cfg(test)]
mod tests {
use std::fs::File;

use anyhow::Result;
use tempfile::tempdir;
use turbopath::AbsoluteSystemPathBuf;

use super::LOCKFILE;
use crate::package_manager::PackageManager;

#[test]
fn test_detect_bun() -> Result<()> {
let repo_root = tempdir()?;
let repo_root_path = AbsoluteSystemPathBuf::try_from(repo_root.path())?;

let lockfile_path = repo_root.path().join(LOCKFILE);
File::create(lockfile_path)?;
let package_manager = PackageManager::detect_package_manager(&repo_root_path)?;
assert_eq!(package_manager, PackageManager::Bun);

Ok(())
}
}
100 changes: 24 additions & 76 deletions crates/turborepo-repository/src/package_manager/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@ use which::which;

use crate::{
discovery,
package_json::PackageJson,
package_manager::{bun::BunDetector, npm::NpmDetector, pnpm::PnpmDetector, yarn::YarnDetector},
package_json::{self, PackageJson},
package_manager::{pnpm::PnpmDetector, yarn::YarnDetector},
};

#[derive(Debug, Deserialize)]
Expand Down Expand Up @@ -273,6 +273,8 @@ pub enum Error {
#[error("globbing error: {0}")]
Wax(Box<wax::BuildError>, #[backtrace] backtrace::Backtrace),
#[error(transparent)]
PackageJson(#[from] package_json::Error),
#[error(transparent)]
Other(#[from] anyhow::Error),
#[error(transparent)]
NoPackageManager(#[from] NoPackageManager),
Expand Down Expand Up @@ -303,6 +305,10 @@ pub enum Error {

#[error("discovering workspace: {0}")]
WorkspaceDiscovery(#[from] discovery::Error),
#[error("missing packageManager field in package.json")]
MissingPackageManager,
#[error("{0} set in packageManager is not a supported package manager")]
UnsupportedPackageManager(String),
}

impl From<std::convert::Infallible> for Error {
Expand Down Expand Up @@ -414,57 +420,24 @@ impl PackageManager {
///
/// TODO: consider if this method should not need an Option, and possibly be
/// a method on PackageJSON
pub fn get_package_manager(
repo_root: &AbsoluteSystemPath,
pkg: Option<&PackageJson>,
) -> Result<Self, Error> {
// We don't surface errors for `read_package_manager` as we can fall back to
// `detect_package_manager`
if let Some(package_json) = pkg {
if let Ok(Some(package_manager)) = Self::read_package_manager(package_json) {
return Ok(package_manager);
}
}

Self::detect_package_manager(repo_root)
pub fn get_package_manager(package_json: &PackageJson) -> Result<Self, Error> {
Self::read_package_manager(package_json)
}

// Attempts to read the package manager from the package.json
fn read_package_manager(pkg: &PackageJson) -> Result<Option<Self>, Error> {
fn read_package_manager(pkg: &PackageJson) -> Result<Self, Error> {
let Some(package_manager) = &pkg.package_manager else {
return Ok(None);
return Err(Error::MissingPackageManager);
};

let (manager, version) = Self::parse_package_manager_string(package_manager)?;
let version = version.parse()?;
let manager = match manager {
"npm" => Some(PackageManager::Npm),
"bun" => Some(PackageManager::Bun),
"yarn" => Some(YarnDetector::detect_berry_or_yarn(&version)?),
"pnpm" => Some(PnpmDetector::detect_pnpm6_or_pnpm(&version)?),
_ => None,
};

Ok(manager)
}

fn detect_package_manager(repo_root: &AbsoluteSystemPath) -> Result<PackageManager, Error> {
let mut detected_package_managers = PnpmDetector::new(repo_root)
.chain(NpmDetector::new(repo_root))
.chain(YarnDetector::new(repo_root))
.chain(BunDetector::new(repo_root))
.collect::<Result<Vec<_>, Error>>()?;

match detected_package_managers.len() {
0 => Err(NoPackageManager.into()),
1 => Ok(detected_package_managers.pop().unwrap()),
_ => {
let managers = detected_package_managers
.iter()
.map(|mgr| mgr.to_string())
.collect();
Err(Error::MultiplePackageManagers { managers })
}
match manager {
"npm" => Ok(PackageManager::Npm),
"bun" => Ok(PackageManager::Bun),
"yarn" => Ok(YarnDetector::detect_berry_or_yarn(&version)?),
"pnpm" => Ok(PnpmDetector::detect_pnpm6_or_pnpm(&version)?),
_ => Err(Error::UnsupportedPackageManager(manager.to_owned())),
}
}

Expand Down Expand Up @@ -805,52 +778,27 @@ mod tests {
..Default::default()
};
let package_manager = PackageManager::read_package_manager(&package_json)?;
assert_eq!(package_manager, Some(PackageManager::Npm));
assert_eq!(package_manager, PackageManager::Npm);

package_json.package_manager = Some("yarn@2.0.0".to_string());
let package_manager = PackageManager::read_package_manager(&package_json)?;
assert_eq!(package_manager, Some(PackageManager::Berry));
assert_eq!(package_manager, PackageManager::Berry);

package_json.package_manager = Some("yarn@1.9.0".to_string());
let package_manager = PackageManager::read_package_manager(&package_json)?;
assert_eq!(package_manager, Some(PackageManager::Yarn));
assert_eq!(package_manager, PackageManager::Yarn);

package_json.package_manager = Some("pnpm@6.0.0".to_string());
let package_manager = PackageManager::read_package_manager(&package_json)?;
assert_eq!(package_manager, Some(PackageManager::Pnpm6));
assert_eq!(package_manager, PackageManager::Pnpm6);

package_json.package_manager = Some("pnpm@7.2.0".to_string());
let package_manager = PackageManager::read_package_manager(&package_json)?;
assert_eq!(package_manager, Some(PackageManager::Pnpm));
assert_eq!(package_manager, PackageManager::Pnpm);

package_json.package_manager = Some("bun@1.0.1".to_string());
let package_manager = PackageManager::read_package_manager(&package_json)?;
assert_eq!(package_manager, Some(PackageManager::Bun));

Ok(())
}

#[test]
fn test_detect_multiple_package_managers() -> Result<(), Error> {
let repo_root = tempdir()?;
let repo_root_path = AbsoluteSystemPathBuf::try_from(repo_root.path())?;

let package_lock_json_path = repo_root.path().join(npm::LOCKFILE);
File::create(&package_lock_json_path)?;
let pnpm_lock_path = repo_root.path().join(pnpm::LOCKFILE);
File::create(pnpm_lock_path)?;

let error = PackageManager::detect_package_manager(&repo_root_path).unwrap_err();
assert_eq!(
error.to_string(),
"We detected multiple package managers in your repository: pnpm, npm. Please remove \
one of them."
);

fs::remove_file(&package_lock_json_path)?;

let package_manager = PackageManager::detect_package_manager(&repo_root_path)?;
assert_eq!(package_manager, PackageManager::Pnpm);
assert_eq!(package_manager, PackageManager::Bun);

Ok(())
}
Expand Down
Loading

0 comments on commit f8bea45

Please sign in to comment.