Skip to content

Commit dd8bd2d

Browse files
authored
refactor: remove lifetime and cwd from WorkspaceRoot (#62)
### TL;DR Refactored `WorkspaceRoot` to use `Arc<AbsolutePath>` for the path field and separated the `cwd` field from the struct. ### What changed? - Modified `WorkspaceRoot` struct to use `Arc<AbsolutePath>` for the `path` field instead of a reference - Removed the `cwd` field from `WorkspaceRoot` and made it a separate return value in `find_workspace_root` - Updated `find_workspace_root` to return a tuple of `(WorkspaceRoot, RelativePathBuf)` instead of just `WorkspaceRoot` - Updated all callers of `find_workspace_root` to handle the new return type - Fixed all references to `workspace_root.path` to properly dereference the Arc when needed ### How to test? - Run the existing test suite to ensure all functionality works as expected - Verify that workspace discovery and package graph loading work correctly - Check that task graph generation functions properly with the new structure ### Why make this change? This change improves memory management by using `Arc` (Atomic Reference Counting) for the workspace path, allowing it to be shared efficiently across different components without lifetime constraints. Separating the `cwd` from the `WorkspaceRoot` struct makes the API more explicit about what belongs to the workspace definition versus contextual information about the current working directory.
1 parent 0ad7fbb commit dd8bd2d

File tree

5 files changed

+48
-40
lines changed

5 files changed

+48
-40
lines changed

crates/vite_task/src/config/workspace.rs

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -52,16 +52,16 @@ impl Workspace {
5252
/// Returns (workspace root, cwd relative to workspace root, current package root relative to workspace root).
5353
fn determine_current_package_path(
5454
original_cwd: &AbsolutePath,
55-
) -> Result<(&AbsolutePath, RelativePathBuf, Option<RelativePathBuf>), Error> {
56-
let WorkspaceRoot { path: workspace_root, cwd, .. } = find_workspace_root(original_cwd)?;
55+
) -> Result<(Arc<AbsolutePath>, RelativePathBuf, Option<RelativePathBuf>), Error> {
56+
let (WorkspaceRoot { path: workspace_root, .. }, cwd) = find_workspace_root(original_cwd)?;
5757
// current package root is None if it can't be found
5858
let Ok(package_root) = find_package_root(original_cwd) else {
5959
return Ok((workspace_root, cwd, None));
6060
};
6161
let current_package_root = package_root.path;
6262

6363
// Get relative path from workspace root to package root
64-
let current_package_root = current_package_root.strip_prefix(workspace_root)?;
64+
let current_package_root = current_package_root.strip_prefix(&*workspace_root)?;
6565
Ok((workspace_root, cwd, current_package_root))
6666
}
6767

@@ -84,7 +84,7 @@ impl Workspace {
8484

8585
pub fn get_cache_path(cwd: &AbsolutePath) -> Result<AbsolutePathBuf, Error> {
8686
let (workspace_root, _, _) = Self::determine_current_package_path(cwd)?;
87-
Ok(Self::get_cache_path_of_workspace(workspace_root))
87+
Ok(Self::get_cache_path_of_workspace(&workspace_root))
8888
}
8989

9090
pub fn partial_load_with_cache_path(
@@ -96,7 +96,7 @@ impl Workspace {
9696
Self::determine_current_package_path(&cwd)?;
9797

9898
let cache_path =
99-
cache_path.unwrap_or_else(|| Self::get_cache_path_of_workspace(workspace_root));
99+
cache_path.unwrap_or_else(|| Self::get_cache_path_of_workspace(&workspace_root));
100100

101101
if !cache_path.as_path().exists()
102102
&& let Some(cache_dir) = cache_path.as_path().parent()
@@ -136,9 +136,9 @@ impl Workspace {
136136
let (workspace_root, cwd, current_package_path) =
137137
Self::determine_current_package_path(&cwd)?;
138138

139-
let package_graph = vite_workspace::discover_package_graph(workspace_root)?;
139+
let package_graph = vite_workspace::discover_package_graph(&*workspace_root)?;
140140
// Load vite-task.json files for all packages
141-
let packages_with_task_jsons = Self::load_vite_task_jsons(&package_graph, workspace_root)?;
141+
let packages_with_task_jsons = Self::load_vite_task_jsons(&package_graph, &workspace_root)?;
142142

143143
// Find root package.json
144144
let mut package_json = None;
@@ -151,7 +151,7 @@ impl Workspace {
151151
}
152152

153153
let cache_path =
154-
cache_path.unwrap_or_else(|| Self::get_cache_path_of_workspace(workspace_root));
154+
cache_path.unwrap_or_else(|| Self::get_cache_path_of_workspace(&workspace_root));
155155

156156
if !cache_path.as_path().exists()
157157
&& let Some(cache_dir) = cache_path.as_path().parent()
@@ -181,7 +181,7 @@ impl Workspace {
181181
&package_graph,
182182
&package_path_to_node,
183183
&mut task_graph_builder,
184-
workspace_root,
184+
&workspace_root,
185185
)?;
186186

187187
// Add topological dependencies if enabled

crates/vite_task_graph/src/lib.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -175,7 +175,7 @@ pub type TaskGraph = DiGraph<TaskNode, TaskDependencyType, TaskIx>;
175175
impl IndexedTaskGraph {
176176
/// Load the task graph from a discovered workspace using the provided config loader.
177177
pub async fn load(
178-
workspace_root: WorkspaceRoot<'_>,
178+
workspace_root: WorkspaceRoot,
179179
config_loader: impl loader::UserConfigLoader,
180180
) -> Result<Self, TaskGraphLoadError> {
181181
let mut task_graph = DiGraph::<TaskNode, TaskDependencyType, TaskIx>::default();

crates/vite_task_graph/tests/snapshots.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -164,10 +164,10 @@ fn run_case(runtime: &Runtime, tmpdir: &AbsolutePath, case_path: &Path) {
164164
let case_stage_path = tmpdir.join(case_name);
165165
copy_dir(case_path, &case_stage_path).unwrap();
166166

167-
let workspace_root = find_workspace_root(&case_stage_path).unwrap();
167+
let (workspace_root, _cwd) = find_workspace_root(&case_stage_path).unwrap();
168168

169169
assert_eq!(
170-
&case_stage_path, workspace_root.path,
170+
&case_stage_path, &*workspace_root.path,
171171
"folder '{}' should be a workspace root",
172172
case_name
173173
);

crates/vite_workspace/src/lib.rs

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -192,13 +192,13 @@ pub type PackageEdgeIndex = EdgeIndex<DefaultIx>;
192192
pub fn discover_package_graph(
193193
cwd: impl AsRef<AbsolutePath>,
194194
) -> Result<DiGraph<PackageInfo, DependencyType, PackageIx>, Error> {
195-
let workspace_root = find_workspace_root(cwd.as_ref())?;
195+
let (workspace_root, _cwd) = find_workspace_root(cwd.as_ref())?;
196196
load_package_graph(&workspace_root)
197197
}
198198

199199
/// Load the package graph from a discovered workspace.
200200
pub fn load_package_graph(
201-
workspace_root: &WorkspaceRoot<'_>,
201+
workspace_root: &WorkspaceRoot,
202202
) -> Result<DiGraph<PackageInfo, DependencyType, PackageIx>, Error> {
203203
let mut graph_builder = PackageGraphBuilder::default();
204204
let workspaces = match &workspace_root.workspace_file {
@@ -215,7 +215,7 @@ pub fn load_package_graph(
215215
let package_json: PackageJson = serde_json::from_reader(file)?;
216216
graph_builder.add_package(
217217
RelativePathBuf::default(),
218-
workspace_root.path.into(),
218+
Arc::clone(&workspace_root.path),
219219
package_json,
220220
);
221221

@@ -225,10 +225,10 @@ pub fn load_package_graph(
225225

226226
let member_globs = WorkspaceMemberGlobs::new(workspaces);
227227
let mut has_root_package = false;
228-
for package_json_path in member_globs.get_package_json_paths(workspace_root.path)? {
228+
for package_json_path in member_globs.get_package_json_paths(&*workspace_root.path)? {
229229
let package_json: PackageJson = serde_json::from_slice(&fs::read(&package_json_path)?)?;
230230
let absolute_path = package_json_path.parent().unwrap();
231-
let Some(package_path) = absolute_path.strip_prefix(workspace_root.path)? else {
231+
let Some(package_path) = absolute_path.strip_prefix(&*workspace_root.path)? else {
232232
return Err(Error::PackageOutsideWorkspace {
233233
package_path: package_json_path,
234234
workspace_root: workspace_root.path.to_absolute_path_buf(),
@@ -246,7 +246,7 @@ pub fn load_package_graph(
246246
let package_json: PackageJson = serde_json::from_slice(&package_json)?;
247247
graph_builder.add_package(
248248
RelativePathBuf::default(),
249-
workspace_root.path.into(),
249+
Arc::clone(&workspace_root.path),
250250
package_json,
251251
);
252252
}

crates/vite_workspace/src/package_manager.rs

Lines changed: 30 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ use std::{
22
fs::File,
33
io::{BufReader, Seek, SeekFrom},
44
path::Path,
5+
sync::Arc,
56
};
67

78
use vite_path::{AbsolutePath, RelativePathBuf};
@@ -60,33 +61,37 @@ pub enum WorkspaceFile {
6061
///
6162
/// If the workspace file is not found, but a package is found, `workspace_file` will be `NonWorkspacePackage` with the `package.json` File.
6263
#[derive(Debug)]
63-
pub struct WorkspaceRoot<'a> {
64+
pub struct WorkspaceRoot {
6465
/// The absolute path of the workspace root directory.
65-
pub path: &'a AbsolutePath,
66-
/// The cwd that the workspace was found from, relative to the workspace root.
67-
pub cwd: RelativePathBuf,
66+
pub path: Arc<AbsolutePath>,
6867
/// The workspace file.
6968
pub workspace_file: WorkspaceFile,
7069
}
7170

7271
/// Find the workspace root directory from the current working directory. `original_cwd` must be absolute.
7372
///
73+
/// Returns the workspace root and the relative path from the workspace root to the original cwd.
74+
///
7475
/// If the workspace file is not found, but a package is found, `workspace_file` will be `NonWorkspacePackage` with the `package.json` File.
7576
///
7677
/// If neither workspace nor package is found, will return `PackageJsonNotFound` error.
77-
pub fn find_workspace_root(original_cwd: &AbsolutePath) -> Result<WorkspaceRoot<'_>, Error> {
78+
pub fn find_workspace_root(
79+
original_cwd: &AbsolutePath,
80+
) -> Result<(WorkspaceRoot, RelativePathBuf), Error> {
7881
let mut cwd = original_cwd;
7982

8083
loop {
8184
// Check for pnpm-workspace.yaml for pnpm workspace
8285
if let Some(file) = open_exists_file(cwd.join("pnpm-workspace.yaml"))? {
83-
return Ok(WorkspaceRoot {
84-
path: cwd,
85-
cwd: original_cwd
86-
.strip_prefix(cwd)?
87-
.expect("cwd must be within the pnpm workspace"),
88-
workspace_file: WorkspaceFile::PnpmWorkspaceYaml(file),
89-
});
86+
let relative_cwd =
87+
original_cwd.strip_prefix(cwd)?.expect("cwd must be within the pnpm workspace");
88+
return Ok((
89+
WorkspaceRoot {
90+
path: Arc::from(cwd),
91+
workspace_file: WorkspaceFile::PnpmWorkspaceYaml(file),
92+
},
93+
relative_cwd,
94+
));
9095
}
9196

9297
// Check for package.json with workspaces field for npm/yarn workspace
@@ -96,11 +101,15 @@ pub fn find_workspace_root(original_cwd: &AbsolutePath) -> Result<WorkspaceRoot<
96101
if package_json.get("workspaces").is_some() {
97102
// Reset the file cursor since we consumed it reading
98103
file.seek(SeekFrom::Start(0))?;
99-
return Ok(WorkspaceRoot {
100-
path: cwd,
101-
cwd: original_cwd.strip_prefix(cwd)?.expect("cwd must be within the workspace"),
102-
workspace_file: WorkspaceFile::NpmWorkspaceJson(file),
103-
});
104+
let relative_cwd =
105+
original_cwd.strip_prefix(cwd)?.expect("cwd must be within the workspace");
106+
return Ok((
107+
WorkspaceRoot {
108+
path: Arc::from(cwd),
109+
workspace_file: WorkspaceFile::NpmWorkspaceJson(file),
110+
},
111+
relative_cwd,
112+
));
104113
}
105114
}
106115

@@ -113,11 +122,10 @@ pub fn find_workspace_root(original_cwd: &AbsolutePath) -> Result<WorkspaceRoot<
113122
// We've reached the root, try to find the package root and return the non-workspace package.
114123
let package_root = find_package_root(original_cwd)?;
115124
let workspace_file = WorkspaceFile::NonWorkspacePackage(package_root.package_json);
116-
return Ok(WorkspaceRoot {
117-
path: package_root.path,
118-
cwd: package_root.cwd,
119-
workspace_file,
120-
});
125+
return Ok((
126+
WorkspaceRoot { path: Arc::from(package_root.path), workspace_file },
127+
package_root.cwd,
128+
));
121129
}
122130
}
123131
}

0 commit comments

Comments
 (0)