Adds support for generating package maps#311
Conversation
✅ Deploy Preview for yarn-v6 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
@cursor review |
⏱️ Benchmark Resultsgatsby install-full-cold
📊 Raw benchmark data (gatsby install-full-cold)Base times: 4.437s, 4.439s, 4.550s, 4.552s, 4.450s, 4.553s, 4.545s, 4.483s, 4.544s, 4.406s, 4.442s, 4.376s, 4.414s, 4.344s, 4.424s, 4.467s, 4.438s, 4.443s, 4.334s, 4.477s, 4.353s, 4.438s, 4.344s, 4.474s, 4.331s, 4.412s, 4.350s, 4.487s, 4.424s, 4.393s Head times: 4.448s, 4.388s, 4.482s, 4.419s, 4.281s, 4.430s, 4.506s, 4.440s, 4.371s, 4.396s, 4.353s, 4.411s, 4.388s, 4.527s, 4.432s, 4.345s, 4.355s, 4.398s, 4.471s, 4.403s, 4.393s, 4.424s, 4.401s, 4.387s, 4.319s, 4.422s, 4.448s, 4.467s, 4.420s, 4.431s gatsby install-cache-only
📊 Raw benchmark data (gatsby install-cache-only)Base times: 1.261s, 1.285s, 1.265s, 1.272s, 1.265s, 1.259s, 1.266s, 1.279s, 1.263s, 1.294s, 1.277s, 1.271s, 1.249s, 1.246s, 1.274s, 1.248s, 1.270s, 1.272s, 1.275s, 1.275s, 1.265s, 1.270s, 1.242s, 1.260s, 1.250s, 1.273s, 1.257s, 1.267s, 1.259s, 1.267s Head times: 1.272s, 1.275s, 1.263s, 1.268s, 1.266s, 1.262s, 1.269s, 1.264s, 1.264s, 1.269s, 1.264s, 1.262s, 1.268s, 1.252s, 1.250s, 1.289s, 1.284s, 1.718s, 1.272s, 1.260s, 1.264s, 1.277s, 1.273s, 1.279s, 1.279s, 1.280s, 1.264s, 1.281s, 1.259s, 1.269s gatsby install-cache-and-lock (warm, with lockfile)
📊 Raw benchmark data (gatsby install-cache-and-lock (warm, with lockfile))Base times: 0.342s, 0.339s, 0.339s, 0.338s, 0.339s, 0.343s, 0.338s, 0.343s, 0.342s, 0.341s, 0.344s, 0.342s, 0.338s, 0.337s, 0.345s, 0.354s, 0.341s, 0.345s, 0.342s, 0.340s, 0.345s, 0.348s, 0.341s, 0.342s, 0.344s, 0.348s, 0.342s, 0.346s, 0.343s, 0.345s Head times: 0.345s, 0.340s, 0.347s, 0.342s, 0.343s, 0.342s, 0.343s, 0.342s, 0.878s, 0.345s, 0.342s, 0.348s, 0.346s, 0.344s, 0.345s, 0.349s, 0.348s, 0.352s, 0.343s, 0.392s, 0.337s, 0.338s, 0.341s, 0.342s, 0.341s, 0.343s, 0.335s, 0.337s, 0.344s, 0.341s |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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.
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
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()?)?; |
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.
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.
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).
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.
Reviewed by Cursor Bugbot for commit 13aa7ed. Configure here.
| install: &Install, | ||
| work_tree: &WorkTree, | ||
| workspace_node_idx: usize, | ||
| package_map_builder: Option<&mut NodeModulesPackageMapBuilder>, |
There was a problem hiding this comment.
Loose maps miss self-reference symlinks
Medium Severity
With nodePackageMapType: loose and nodeExperimentalPackageMap enabled, workspace self-reference symlinks created in register_workspace_symlinks_at are never passed to NodeModulesPackageMapBuilder, so those node_modules entries are missing from the package map index. Requires that work without a map can fail under loose maps even though the symlink layout is present.
Reviewed by Cursor Bugbot for commit 13aa7ed. Configure here.
2 participants
Counterpart of yarnpkg/berry#7184 for the Rust implementation.
Note
Medium Risk
Changes core install/link output and
NODE_OPTIONSfor all scripts when enabled; resolution enforcement can break packages that relied on undeclared hoisted deps untillooseis used.Overview
Adds Node.js experimental package map support for
node-modulesandpnpmlinkers (Rust ZPM), mirroring the Berry work.Configuration: New settings
nodeExperimentalPackageMap(opt-in injection intoNODE_OPTIONS) andnodePackageMapType(standardvsloosedependency edges). Documented in schema andwebsite/config/yarnrc.json.Install output: Linkers write
node_modules/.package-map.json(per workspace / island under that workspace’snode_modules). A newlinker/package_mapmodule builds maps from the installed layout—standard limitsrequireto declared deps (plus resolution/package extensions); loose follows hoistednode_modulesvisibility. PnP installs remove any root package map artifact on cleanup.Scripts: When
nodeExperimentalPackageMapis enabled, script envs append--experimental-package-map=…toNODE_OPTIONS(island-aware cwd), and strip stale flags when PnP loaders are removed.Tests: New
packageMaps.test.tsand island/script coverage;node_moduleslisting expects.package-map.json.Reviewed by Cursor Bugbot for commit 13aa7ed. Bugbot is set up for automated code reviews on this repo. Configure here.