-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
feat: require packageManager in package.json (#8017)
### 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
1 parent
d850428
commit 5966983
Showing
20 changed files
with
99 additions
and
419 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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": "[email protected]"}"#, | ||
) | ||
.unwrap(); | ||
repo_root | ||
.join_component("package-lock.json") | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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": "[email protected]"}"#, | ||
) | ||
.unwrap(); | ||
|
||
let watcher = FileSystemWatcher::new_with_default_cookie_dir(&repo_root).unwrap(); | ||
|
@@ -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": "[email protected]"}"#, | ||
) | ||
.unwrap(); | ||
|
||
let watcher = FileSystemWatcher::new_with_default_cookie_dir(&repo_root).unwrap(); | ||
|
@@ -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": "[email protected]"}"#, | ||
) | ||
.unwrap(); | ||
|
||
let mut data = package_watcher.discover_packages_blocking().await.unwrap(); | ||
|
@@ -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": "[email protected]"}"#) | ||
.create_with_contents(r#"{"packageManager": "[email protected].0"}"#) | ||
.unwrap(); | ||
repo_root | ||
.join_component("pnpm-lock.yaml") | ||
|
@@ -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": "[email protected]"}"#) | ||
.create_with_contents(r#"{"packageManager": "[email protected].0"}"#) | ||
.unwrap(); | ||
repo_root | ||
.join_component("package-lock.json") | ||
|
@@ -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": "[email protected].0", "workspaces": ["foo/*"]}"#) | ||
.unwrap(); | ||
|
||
let resp = package_watcher.discover_packages_blocking().await.unwrap(); | ||
|
@@ -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": "[email protected].0", "workspaces": ["foo/***"]}"#) | ||
.unwrap(); | ||
|
||
// We expect an error due to invalid workspace glob | ||
|
@@ -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": "[email protected].0", "workspaces": ["foo/*"]}"#) | ||
.unwrap(); | ||
let resp = package_watcher.discover_packages_blocking().await.unwrap(); | ||
assert_eq!(resp.package_manager, PackageManager::Npm); | ||
|
@@ -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": "[email protected]"}"#) | ||
.create_with_contents(r#"{"packageManager": "[email protected].0"}"#) | ||
.unwrap(); | ||
let pnpm_lock_file = repo_root.join_component("pnpm-lock.yaml"); | ||
pnpm_lock_file.create_with_contents("").unwrap(); | ||
|
@@ -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 | ||
|
@@ -980,7 +986,7 @@ mod test { | |
|
||
// update package.json to complete the transition | ||
root_package_json_path | ||
.create_with_contents(r#"{"packageManager": "[email protected]", "workspaces": ["foo/*"]}"#) | ||
.create_with_contents(r#"{"packageManager": "[email protected].0", "workspaces": ["foo/*"]}"#) | ||
.unwrap(); | ||
let resp = package_watcher.discover_packages_blocking().await.unwrap(); | ||
assert_eq!(resp.package_manager, PackageManager::Npm); | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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() | ||
|
@@ -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\": \"[email protected]\"}", | ||
) | ||
.unwrap(); | ||
monorepo_root | ||
.join_component("package-lock.json") | ||
|
@@ -188,7 +189,9 @@ mod test { | |
.unwrap(); | ||
standalone_monorepo | ||
.join_component("package.json") | ||
.create_with_contents("{\"workspaces\": [\"packages/*\"]}") | ||
.create_with_contents( | ||
"{\"workspaces\": [\"packages/*\"], \"packageManager\": \"[email protected]\"}", | ||
) | ||
.unwrap(); | ||
standalone_monorepo | ||
.join_component("package-lock.json") | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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(()) | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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)] | ||
|
@@ -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), | ||
|
@@ -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 { | ||
|
@@ -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())), | ||
} | ||
} | ||
|
||
|
@@ -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("[email protected]".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("[email protected]".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("[email protected]".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("[email protected]".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("[email protected]".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(()) | ||
} | ||
|
Oops, something went wrong.