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.

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

feat: update @turbo/repository to use new terminology #7078

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
23 changes: 9 additions & 14 deletions packages/turbo-repository/__tests__/find.test.ts
Original file line number Diff line number Diff line change
@@ -1,27 +1,22 @@
import * as path from "node:path";
import {
PackageManagerRoot,
Package,
PackageManager,
} from "../js/dist/index.js";
import { Workspace, Package, PackageManager } from "../js/dist/index.js";

describe("find", () => {
it("finds a package manager root", async () => {
const packageManagerRoot = await PackageManagerRoot.find();
console.log(packageManagerRoot);
describe("Workspace", () => {
it("finds a workspace", async () => {
const workspace = await Workspace.find();
const expectedRoot = path.resolve(__dirname, "../../..");
expect(packageManagerRoot.root).toBe(expectedRoot);
expect(workspace.absolutePath).toBe(expectedRoot);
});

it("enumerates packages", async () => {
const packageManagerRoot = await PackageManagerRoot.find();
const packages: Package[] = await packageManagerRoot.packages();
const workspace = await Workspace.find();
const packages: Package[] = await workspace.findPackages();
expect(packages.length).not.toBe(0);
});

it("finds a package manager", async () => {
const packageManagerRoot = await PackageManagerRoot.find();
const packageManager: PackageManager = packageManagerRoot.packageManager();
const workspace = await Workspace.find();
const packageManager: PackageManager = workspace.packageManager();
expect(packageManager.name).toBe("pnpm");
});
// TODO: proper tests on real fixtures
Expand Down
11 changes: 5 additions & 6 deletions packages/turbo-repository/js/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,17 +3,16 @@

/* auto-generated by NAPI-RS */

export class PackageManagerRoot {
readonly root: string;
readonly isSinglePackage: boolean;
static find(path?: string | undefined | null): Promise<PackageManagerRoot>;
export class Workspace {
readonly absolutePath: string;
readonly isMultiPackage: boolean;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the default is a single package, it feels better to look for the opt-in to a multi-package workspace.

static find(path?: string | undefined | null): Promise<Workspace>;
packageManager(): PackageManager;
packages(): Promise<Array<Package>>;
findPackages(): Promise<Array<Package>>;
}
export class PackageManager {
readonly name: string;
}
export class Package {
readonly absolutePath: string;
readonly repoPath: string;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't think of a good reason to also return the workspace path here, as it's in the parent class.

}
44 changes: 22 additions & 22 deletions packages/turbo-repository/rust/src/internal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,11 @@ use napi::Status;
use thiserror::Error;
use turbopath::{AbsoluteSystemPathBuf, PathError};
use turborepo_repository::{
inference::{self, RepoMode, RepoState},
inference::{self, RepoMode as WorkspaceType, RepoState as WorkspaceState},
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the glossary we refer to the workspace type, not mode, and I wanted to align on that here if possible.

package_manager,
};

use crate::{Package, PackageManagerRoot};
use crate::{Package, Workspace};

/// This module is used to isolate code with defined errors
/// from code in lib.rs that needs to have errors coerced to strings /
Expand All @@ -27,10 +27,10 @@ pub(crate) enum Error {
error: String,
path: AbsoluteSystemPathBuf,
},
#[error("Failed to discover packages from root {repo_root}: {error}")]
#[error("Failed to discover packages from root {workspace_root}: {error}")]
PackageJsons {
error: package_manager::Error,
repo_root: AbsoluteSystemPathBuf,
workspace_root: AbsoluteSystemPathBuf,
},
#[error("Package directory {0} has no parent")]
MissingParent(AbsoluteSystemPathBuf),
Expand All @@ -42,7 +42,7 @@ impl From<Error> for napi::Error<Status> {
}
}

impl PackageManagerRoot {
impl Workspace {
pub(crate) async fn find_internal(path: Option<String>) -> Result<Self, Error> {
let reference_dir = path
.map(|path| {
Expand All @@ -57,12 +57,12 @@ impl PackageManagerRoot {
path_error,
})
})?;
let repo_state = RepoState::infer(&reference_dir)?;
let is_monorepo = repo_state.mode == RepoMode::MultiPackage;
let workspace_state = WorkspaceState::infer(&reference_dir)?;
let is_multi_package = workspace_state.mode == WorkspaceType::MultiPackage;
Ok(Self {
root: repo_state.root.to_string(),
repo_state,
is_single_package: !is_monorepo,
absolute_path: workspace_state.root.to_string(),
workspace_state,
is_multi_package,
})
}

Expand All @@ -71,28 +71,28 @@ impl PackageManagerRoot {
// manager discovery. That probably isn't the best design. We should
// address it when we decide how we want to handle possibly finding a
// repo root but not finding a package manager.
let package_manager =
self.repo_state
.package_manager
.as_ref()
.map_err(|error| Error::PackageManager {
error: error.to_string(),
path: self.repo_state.root.clone(),
})?;
let package_manager = self
.workspace_state
.package_manager
.as_ref()
.map_err(|error| Error::PackageManager {
error: error.to_string(),
path: self.workspace_state.root.clone(),
})?;
let package_manager = *package_manager;
let repo_root = self.repo_state.root.clone();
let workspace_root = self.workspace_state.root.clone();
let package_json_paths =
tokio::task::spawn(async move { package_manager.get_package_jsons(&repo_root) })
tokio::task::spawn(async move { package_manager.get_package_jsons(&workspace_root) })
.await
.expect("package enumeration should not crash")
.map_err(|error| Error::PackageJsons {
error,
repo_root: self.repo_state.root.clone(),
workspace_root: self.workspace_state.root.clone(),
})?;
let packages = package_json_paths
.map(|path| {
path.parent()
.map(|package_path| Package::new(&self.repo_state.root, package_path))
.map(|package_path| Package::new(&self.workspace_state.root, package_path))
.ok_or_else(|| Error::MissingParent(path.to_owned()))
})
.collect::<Result<Vec<Package>, Error>>()?;
Expand Down
29 changes: 13 additions & 16 deletions packages/turbo-repository/rust/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,18 +1,18 @@
use napi_derive::napi;
use turbopath::AbsoluteSystemPath;
use turborepo_repository::{
inference::RepoState, package_manager::PackageManager as RustPackageManager,
inference::RepoState as WorkspaceState, package_manager::PackageManager as RustPackageManager,
};

mod internal;

#[napi]
pub struct PackageManagerRoot {
repo_state: RepoState,
pub struct Workspace {
workspace_state: WorkspaceState,
#[napi(readonly)]
pub root: String,
pub absolute_path: String,
#[napi(readonly)]
pub is_single_package: bool,
pub is_multi_package: bool,
}

#[napi]
Expand All @@ -27,18 +27,15 @@ pub struct PackageManager {
pub struct Package {
#[napi(readonly)]
pub absolute_path: String,
#[napi(readonly)]
pub repo_path: String,
}

impl Package {
fn new(repo_root: &AbsoluteSystemPath, workspace_path: &AbsoluteSystemPath) -> Self {
let repo_path = repo_root
.anchor(workspace_path)
fn new(workspace_path: &AbsoluteSystemPath, package_path: &AbsoluteSystemPath) -> Self {
workspace_path
.anchor(package_path)
.expect("workspace is in the repo root");
Self {
absolute_path: workspace_path.to_string(),
repo_path: repo_path.to_string(),
absolute_path: package_path.to_string(),
}
}
}
Expand All @@ -53,9 +50,9 @@ impl From<RustPackageManager> for PackageManager {
}

#[napi]
impl PackageManagerRoot {
impl Workspace {
#[napi(factory)]
pub async fn find(path: Option<String>) -> Result<PackageManagerRoot, napi::Error> {
pub async fn find(path: Option<String>) -> Result<Workspace, napi::Error> {
Self::find_internal(path).await.map_err(|e| e.into())
}

Expand All @@ -64,14 +61,14 @@ impl PackageManagerRoot {
// match rather than map/map_err due to only the Ok variant implementing "Copy"
// match lets us handle each case independently, rather than forcing the whole
// value to a reference or concrete value
match self.repo_state.package_manager.as_ref() {
match self.workspace_state.package_manager.as_ref() {
Ok(pm) => Ok((*pm).into()),
Err(e) => Err(napi::Error::from_reason(format!("{}", e))),
}
}

#[napi]
pub async fn packages(&self) -> std::result::Result<Vec<Package>, napi::Error> {
pub async fn find_packages(&self) -> std::result::Result<Vec<Package>, napi::Error> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the thought behind adding the find that it better indicates that this function does work and isn't just a getter?

Copy link
Contributor Author

@mrmckeb mrmckeb Jan 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. My understanding was that this async function was calling package_manager.get_package_jsons() underneath, which traverses. But I might have misunderstood the code here.

Otherwise I might have suggested getPackages - or that it was a key called packages with an array value.

self.packages_internal().await.map_err(|e| e.into())
}
}
Loading