Adds support for generating package maps#311
Merged
Merged
Conversation
✅ Deploy Preview for yarn-v6 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Member
Author
|
@cursor review |
⏱️ Benchmark Resultsgatsby install-full-cold
📊 Raw benchmark data (gatsby install-full-cold)Base times: 4.464s, 4.465s, 4.411s, 4.533s, 4.398s, 4.459s, 4.461s, 4.434s, 4.536s, 4.411s, 4.441s, 4.425s, 4.441s, 4.520s, 4.542s, 4.540s, 4.510s, 4.406s, 4.420s, 4.505s, 4.505s, 4.515s, 4.447s, 4.396s, 4.492s, 4.570s, 4.498s, 4.513s, 4.400s, 4.514s Head times: 4.506s, 4.501s, 4.567s, 4.544s, 4.463s, 4.465s, 4.449s, 4.533s, 4.541s, 4.460s, 4.470s, 4.537s, 4.451s, 4.453s, 4.501s, 4.497s, 4.420s, 4.528s, 4.492s, 4.555s, 4.475s, 4.509s, 4.467s, 4.436s, 4.470s, 4.536s, 4.440s, 4.276s, 4.363s, 4.418s gatsby install-cache-only
📊 Raw benchmark data (gatsby install-cache-only)Base times: 1.311s, 1.304s, 1.281s, 1.313s, 1.293s, 1.312s, 1.286s, 1.303s, 1.319s, 1.276s, 1.295s, 1.290s, 1.341s, 1.285s, 1.291s, 1.283s, 1.296s, 1.289s, 1.614s, 1.313s, 1.304s, 1.301s, 1.304s, 1.296s, 1.348s, 1.304s, 1.285s, 1.297s, 1.297s, 1.289s Head times: 1.320s, 1.308s, 1.272s, 1.287s, 1.310s, 1.310s, 1.322s, 1.304s, 1.296s, 1.272s, 1.297s, 1.299s, 1.302s, 1.287s, 1.305s, 1.311s, 1.293s, 1.319s, 1.313s, 1.756s, 1.304s, 1.303s, 1.285s, 1.324s, 1.283s, 1.305s, 1.329s, 1.312s, 1.283s, 1.294s gatsby install-cache-and-lock (warm, with lockfile)
📊 Raw benchmark data (gatsby install-cache-and-lock (warm, with lockfile))Base times: 0.350s, 0.356s, 0.351s, 0.351s, 0.353s, 0.352s, 0.350s, 0.349s, 0.359s, 0.356s, 0.356s, 0.356s, 0.355s, 0.360s, 0.356s, 0.359s, 0.359s, 0.356s, 0.355s, 0.354s, 0.351s, 0.361s, 0.354s, 0.355s, 0.356s, 0.351s, 0.351s, 0.362s, 0.359s, 0.358s Head times: 0.356s, 0.354s, 0.352s, 0.350s, 0.353s, 0.351s, 0.353s, 0.831s, 0.352s, 0.350s, 0.354s, 0.353s, 0.352s, 0.351s, 0.356s, 0.355s, 0.355s, 0.356s, 0.354s, 0.354s, 0.352s, 0.352s, 0.350s, 0.352s, 0.354s, 0.352s, 0.351s, 0.354s, 0.351s, 0.359s |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Nm map lacks self dependencies
- Added implicit self-dependency to node-modules package map builder to mirror pnpm linker behavior, ensuring packages can resolve their own name when using nodeExperimentalPackageMap.
Or push these changes by commenting:
@cursor push ba99b5b726
Preview (ba99b5b726)
diff --git a/packages/zpm/src/linker/package_map.rs b/packages/zpm/src/linker/package_map.rs
--- a/packages/zpm/src/linker/package_map.rs
+++ b/packages/zpm/src/linker/package_map.rs
@@ -267,9 +267,16 @@
let tree
= &install.install_state.resolution_tree;
- resolution_dependency_names(tree, locator)
+ let mut dependency_names = resolution_dependency_names(tree, locator)
.or_else(|| workspace_link_dependency_names(project, tree, locator))
- .unwrap_or_default()
+ .unwrap_or_default();
+
+ // Add implicit self-dependency for non-workspace packages when there's no explicit self-dependency
+ if !locator.reference.is_workspace_reference() && !dependency_names.contains(locator.ident.as_str()) {
+ dependency_names.insert(locator.ident.as_str().to_string());
+ }
+
+ dependency_names
}
fn resolution_dependency_names(tree: &ResolutionTree, locator: &Locator) -> Option<BTreeSet<String>> {You can send follow-ups to the cloud agent here.
Member
Author
When nodeExperimentalPackageMap is enabled, packages that rely on a self symlink can now correctly resolve their own name under standard enforcement. The node-modules package map builder now adds an implicit self-dependency for non-workspace packages when there's no explicit self-dependency, mirroring the behavior of the pnpm linker. This ensures parity between both linkers' package map generation within the same PR. Applied via @cursor push command
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Map saved before island linking
- Moved package map persistence to after island linking and added logic to rebuild the map including island packages when islands are present.
- ✅ Fixed: Stale map after build failure
- Added removal of old package map file at the start of node-modules linking to prevent stale maps from persisting after build failures.
Or push these changes by commenting:
@cursor push e6a5becc1a
Preview (e6a5becc1a)
diff --git a/packages/zpm/src/linker/mod.rs b/packages/zpm/src/linker/mod.rs
--- a/packages/zpm/src/linker/mod.rs
+++ b/packages/zpm/src/linker/mod.rs
@@ -22,6 +22,7 @@
pub struct LinkResult {
pub packages_by_location: BTreeMap<Path, Locator>,
pub build_requests: BuildRequests,
+ pub package_map: Option<package_map::PackageMap>,
}
pub async fn link_project<'a>(project: &'a Project, install: &'a Install) -> Result<LinkResult, Error> {
@@ -38,6 +39,8 @@
=> pnpm::link_project_pnpm(project, install).await?,
};
+ let has_islands = !install.resolved_islands.is_empty();
+
// Per-island link steps
for island in &install.resolved_islands {
match island.linker {
@@ -55,9 +58,88 @@
}
}
+ // Persist package map after all islands have been linked
+ // If islands were present, rebuild the package map to include island packages
+ if let Some(package_map) = &result.package_map {
+ if has_islands && matches!(project.config.settings.node_linker.value, NodeLinker::NodeModules | NodeLinker::Pnpm) {
+ // Rebuild the package map to include island-added packages
+ let rebuilt_package_map = rebuild_package_map_with_islands(project, install, &result.packages_by_location)?;
+ package_map::persist_package_map(project, &rebuilt_package_map)?;
+ } else {
+ package_map::persist_package_map(project, package_map)?;
+ }
+ }
+
Ok(result)
}
+fn rebuild_package_map_with_islands(
+ project: &Project,
+ install: &Install,
+ packages_by_location: &BTreeMap<Path, Locator>,
+) -> Result<package_map::PackageMap, Error> {
+ match project.config.settings.node_linker.value {
+ NodeLinker::NodeModules => {
+ let mut builder = package_map::NodeModulesPackageMapBuilder::new(project, install);
+
+ // Register all packages from the final packages_by_location map
+ for (rel_path, locator) in packages_by_location {
+ let location_abs = project.project_cwd.with_join(rel_path);
+
+ // Determine the package_path (where the actual package files are)
+ let package_path = match install.package_data.get(&locator.physical_locator()) {
+ Some(crate::fetchers::PackageData::Local { package_directory, .. }) => {
+ package_directory.clone()
+ },
+ _ => location_abs.clone(),
+ };
+
+ builder.register_package(location_abs, package_path, locator);
+ }
+
+ builder.build()
+ },
+ NodeLinker::Pnpm => {
+ let mut builder = package_map::PnpmPackageMapBuilder::new(project);
+ let tree = &install.install_state.resolution_tree;
+
+ // Register all packages
+ for locator in packages_by_location.values() {
+ let package_location = packages_by_location
+ .iter()
+ .find(|(_, l)| *l == locator)
+ .map(|(path, _)| project.project_cwd.with_join(path))
+ .unwrap();
+
+ builder.register_package(locator, package_location);
+ }
+
+ // Register dependencies
+ for (locator, resolution) in &tree.locator_resolutions {
+ for (dep_name, descriptor) in &resolution.dependencies {
+ if let Some(dep_locator) = tree.descriptor_to_locator.get(descriptor) {
+ builder.register_dependency(locator, dep_name, dep_locator)?;
+ }
+ }
+
+ // Add implicit self-dependency for non-workspace packages
+ if !locator.reference.is_workspace_reference() {
+ let has_explicit_self = resolution.dependencies.contains_key(&locator.ident);
+ if !has_explicit_self {
+ builder.register_dependency(locator, &locator.ident, locator)?;
+ }
+ }
+ }
+
+ builder.build()
+ },
+ _ => {
+ // Other linkers don't use package maps
+ Err(Error::Unsupported)
+ },
+ }
+}
+
fn cleanup_inactive_linker_artifacts(project: &Project) -> Result<(), Error> {
let active = project.config.settings.node_linker.value;
diff --git a/packages/zpm/src/linker/nm/mod.rs b/packages/zpm/src/linker/nm/mod.rs
--- a/packages/zpm/src/linker/nm/mod.rs
+++ b/packages/zpm/src/linker/nm/mod.rs
@@ -5,7 +5,7 @@
use zpm_utils::{FromFileString, IoResultExt, Path, ToHumanString};
use crate::{
- build::{self, BuildRequest, BuildRequests}, content_flags, error::Error, fetchers::PackageData, install::Install, linker::{self, LinkResult, helpers::PackageMeta, nm::hoist::{Hoister, WorkTree}, package_map::{NodeModulesPackageMapBuilder, persist_package_map}}, project::Project
+ build::{self, BuildRequest, BuildRequests}, content_flags, error::Error, fetchers::PackageData, install::Install, linker::{self, LinkResult, helpers::PackageMeta, nm::hoist::{Hoister, WorkTree}, package_map::NodeModulesPackageMapBuilder}, project::Project
};
pub mod hoist;
@@ -573,6 +573,7 @@
Ok(LinkResult {
packages_by_location,
build_requests,
+ package_map: None,
})
}
@@ -789,6 +790,11 @@
pub async fn link_project_nm(project: &Project, install: &Install) -> Result<LinkResult, Error> {
warn_about_portals_if_any(install);
+ // Remove stale package map to prevent it from persisting after a build failure
+ project.package_map_path()
+ .fs_rm()
+ .ok_missing()?;
+
let mut work_tree
= WorkTree::new(project, &install.install_state);
@@ -833,7 +839,7 @@
run_cas_extractions(project, install, &cas_extractions)?;
- persist_package_map(project, &package_map_builder.build()?)?;
+ let package_map = package_map_builder.build()?;
let dependencies_meta
= linker::helpers::TopLevelConfiguration::from_project(project);
@@ -849,5 +855,6 @@
Ok(LinkResult {
packages_by_location,
build_requests,
+ package_map: Some(package_map),
})
}
diff --git a/packages/zpm/src/linker/pnp.rs b/packages/zpm/src/linker/pnp.rs
--- a/packages/zpm/src/linker/pnp.rs
+++ b/packages/zpm/src/linker/pnp.rs
@@ -593,5 +593,6 @@
Ok(LinkResult {
packages_by_location,
build_requests,
+ package_map: None,
})
}
diff --git a/packages/zpm/src/linker/pnpm.rs b/packages/zpm/src/linker/pnpm.rs
--- a/packages/zpm/src/linker/pnpm.rs
+++ b/packages/zpm/src/linker/pnpm.rs
@@ -8,7 +8,7 @@
error::Error,
fetchers::PackageData,
install::Install,
- linker::{self, LinkResult, package_map::{PnpmPackageMapBuilder, persist_package_map}},
+ linker::{self, LinkResult, package_map::PnpmPackageMapBuilder},
project::Project,
tree_resolver::ResolutionTree,
};
@@ -329,7 +329,7 @@
}
}
- persist_package_map(project, &package_map_builder.build()?)?;
+ let package_map = package_map_builder.build()?;
let package_build_dependencies = linker::helpers::populate_build_entry_dependencies(
&package_build_entries,
@@ -345,5 +345,6 @@
Ok(LinkResult {
packages_by_location,
build_requests,
+ package_map: Some(package_map),
})
}
diff --git a/packages/zpm/src/linker/venv.rs b/packages/zpm/src/linker/venv.rs
--- a/packages/zpm/src/linker/venv.rs
+++ b/packages/zpm/src/linker/venv.rs
@@ -180,5 +180,6 @@
entries: vec![],
dependencies: BTreeMap::new(),
},
+ package_map: None,
})
}You can send follow-ups to the cloud agent here.
|
|
||
| run_cas_extractions(project, install, &cas_extractions)?; | ||
|
|
||
| persist_package_map(project, &package_map_builder.build()?)?; |
Contributor
There was a problem hiding this comment.
Stale map after build failure
Medium Severity
The node-modules linker updates node_modules while generating workspaces, but writes .package-map.json only after package_map_builder.build() succeeds. Unlike pnpm, it does not remove the old map at link start, so a failed build leaves a previous map on disk that may not match the new layout.
Reviewed by Cursor Bugbot for commit 6eadee7. Configure here.
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Island maps skip NODE_OPTIONS injection
- Modified ScriptEnvironment::with_package to detect island workspaces with node-modules linker and inject the workspace-local package map path instead of the root package map.
Or push these changes by commenting:
@cursor push 4869fa7c37
Preview (4869fa7c37)
diff --git a/packages/zpm/src/script.rs b/packages/zpm/src/script.rs
--- a/packages/zpm/src/script.rs
+++ b/packages/zpm/src/script.rs
@@ -749,6 +749,40 @@
self.binaries = self.binaries
.with_package(&binaries, &project.project_cwd)?;
+ // If this package is a workspace in an island with node-modules linker,
+ // update NODE_OPTIONS to use the workspace-local package map instead of
+ // the root package map.
+ if project.config.settings.node_experimental_package_map.value {
+ if let Ok(workspace) = project.workspace_by_ident(&locator.ident) {
+ let in_nm_island = project.config.settings.unstable_islands
+ .values()
+ .any(|island| {
+ island.linker.value == zpm_config::IslandLinker::NodeModules
+ && island.workspaces.iter().any(|glob| glob.value.check(&workspace.name))
+ });
+
+ if in_nm_island {
+ let workspace_package_map_path = workspace.path
+ .with_join_str("node_modules")
+ .with_join_str(crate::project::PACKAGE_MAP_NAME);
+
+ if workspace_package_map_path.if_exists().is_some() {
+ // Remove any existing package-map option from NODE_OPTIONS
+ if let Some(Some(node_options)) = self.env.get_mut("NODE_OPTIONS") {
+ let updated = PACKAGE_MAP_MATCHER.replace_all(node_options, "").to_string();
+ *node_options = updated;
+ }
+
+ // Add the workspace-local package map
+ self.append_env("NODE_OPTIONS", ' ', &format!(
+ "--experimental-package-map={}",
+ quote_path_if_needed(&workspace_package_map_path.to_file_string())
+ ));
+ }
+ }
+ }
+ }
+
Ok(self)
}You can send follow-ups to the cloud agent here.
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Loose maps miss self-reference symlinks
- Added package_map_builder.register_package() calls in register_workspace_symlinks_at to ensure workspace self-reference symlinks are indexed in package_locations_by_node_modules_path for loose mode resolution.
Or push these changes by commenting:
@cursor push aeec360c22
Preview (aeec360c22)
diff --git a/packages/zpm/src/linker/nm/mod.rs b/packages/zpm/src/linker/nm/mod.rs
--- a/packages/zpm/src/linker/nm/mod.rs
+++ b/packages/zpm/src/linker/nm/mod.rs
@@ -89,6 +89,7 @@
host_node: &hoist::WorkNode,
host_abs_path: &Path,
candidate_workspaces: impl IntoIterator<Item = (Ident, Path)>,
+ mut package_map_builder: Option<&mut NodeModulesPackageMapBuilder>,
) -> Result<(), Error> {
let global_default = project.config.settings.nm_self_references.value;
let host_children = host_node.children.as_ref();
@@ -147,9 +148,20 @@
= workspace_dir
.relative_to(&host_abs_path.with_join(&symlink_path).dirname().unwrap_or_default());
+ let abs_path
+ = host_abs_path.with_join(&symlink_path);
+
workspace_nm_tree.register_entry(symlink_path, SyncItem::Symlink {
target_path,
})?;
+
+ if let Some(package_map_builder) = package_map_builder.as_deref_mut() {
+ package_map_builder.register_package(
+ abs_path,
+ workspace_dir.clone(),
+ &target_locator,
+ );
+ }
}
Ok(())
@@ -270,6 +282,7 @@
&work_tree.nodes[workspace_node_idx],
&workspace_abs_path,
candidate_workspaces,
+ package_map_builder.as_deref_mut(),
)?;
}You can send follow-ups to the cloud agent here.
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 3 total unresolved issues (including 1 from previous review).
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Exec cwd skips map refresh
- Added with_cwd_and_refresh method to refresh the package map after the final cwd change in yarn exec, ensuring NODE_OPTIONS contains the correct --experimental-package-map flag for the actual working directory.
- ✅ Fixed: Link path workspace lookup fails
- Normalized link paths to absolute before comparing with workspace.path, allowing relative link: targets to correctly match workspace packages in standard package maps.
Or push these changes by commenting:
@cursor push 9ab153219b
Preview (9ab153219b)
diff --git a/packages/zpm/src/commands/exec.rs b/packages/zpm/src/commands/exec.rs
--- a/packages/zpm/src/commands/exec.rs
+++ b/packages/zpm/src/commands/exec.rs
@@ -33,7 +33,7 @@
Ok(ScriptEnvironment::new()?
.with_project(&project)
.with_package(&project, &project.active_package()?)?
- .with_cwd(Path::current_dir()?)
+ .with_cwd_and_refresh(&project, Path::current_dir()?)
.enable_shell_forwarding()
.enable_signal_delegation()
.run_script(&self.script, &self.args)
diff --git a/packages/zpm/src/linker/package_map.rs b/packages/zpm/src/linker/package_map.rs
--- a/packages/zpm/src/linker/package_map.rs
+++ b/packages/zpm/src/linker/package_map.rs
@@ -303,9 +303,15 @@
let link_path
= Path::from_str(¶ms.path).ok()?;
+ let link_path_abs = if link_path.is_absolute() {
+ link_path
+ } else {
+ project.project_cwd.with_join(&link_path)
+ };
+
let workspace = project.workspaces
.iter()
- .find(|workspace| workspace.path == link_path)?;
+ .find(|workspace| workspace.path == link_path_abs)?;
resolution_dependency_names(tree, &workspace.locator())
}
diff --git a/packages/zpm/src/script.rs b/packages/zpm/src/script.rs
--- a/packages/zpm/src/script.rs
+++ b/packages/zpm/src/script.rs
@@ -801,6 +801,12 @@
self
}
+ pub fn with_cwd_and_refresh(mut self, project: &Project, cwd: Path) -> Self {
+ self.cwd = cwd;
+ self.refresh_package_map(project);
+ self
+ }
+
fn install_binaries(&mut self) -> Result<Path, Error> {
let hash
= Hash64::from_string(&JsonDocument::to_string(&self.binaries)?);You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit 578c179. Configure here.
| .with_join(package_cwd_rel); | ||
|
|
||
| self.attach_package_variables(project, locator)?; | ||
| self.refresh_package_map(project); |
Contributor
There was a problem hiding this comment.
Exec cwd skips map refresh
Medium Severity
yarn exec calls with_package (which sets NODE_OPTIONS from refresh_package_map using that step’s cwd) and then with_cwd to the shell directory without refreshing the package map again, so the injected --experimental-package-map can reflect the wrong working directory when those paths differ.
Reviewed by Cursor Bugbot for commit 578c179. Configure here.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Counterpart of yarnpkg/berry#7184 for the Rust implementation.
Note
Medium Risk
Touches install linking, generated artifacts, and
NODE_OPTIONSfor all scripts; behavior changes module resolution when package maps are enabled, though defaults keep the feature off.Overview
Adds Node.js experimental package map support for
nodeLinker: node-modulesandnodeLinker: pnpm(Rust zpm counterpart to Berry #7184).New settings
nodeExperimentalPackageMap(inject--experimental-package-map=…intoNODE_OPTIONSwhen enabled) andnodePackageMapType(standardvsloosedependency edges). During link, the node-modules and pnpm linkers build and writenode_modules/.package-map.json(per-workspace for node-modules islands), tracking installed layout and declared vs hoisted dependencies.scriptenvironments refresh/strip package-map flags alongside PnP loaders, using island-aware map paths when cwd is in a node-modules island.Supporting changes: new
linker/package_mapmodule,Project::package_map_path, PnP linker cleanup removes stale maps,Path::without_trailing_separators, and acceptance tests for maps, loose mode, and island injection.Reviewed by Cursor Bugbot for commit 889778e. Bugbot is set up for automated code reviews on this repo. Configure here.