Skip to content

Commit 4acfa5d

Browse files
divybotlittledivy
andauthored
fix(npm): run workspace package lifecycle scripts (#34615)
## Summary - Collect `package.json` workspace members as lifecycle-capable install roots, including members without a package `name`/`version`. - Register those workspace packages with the existing lifecycle script runner for both isolated and hoisted `node_modules` layouts. - Order workspace lifecycle scripts so transitive dependencies running *through* non-script workspace packages are still executed before their scripted dependents (addresses review feedback). - Add a regression spec for a workspace member `postinstall` script writing a file, including a script package depending through a non-script workspace package onto another script package. ## Verification - CI is green on this PR head (build + integration + node_compat across all platforms). - `cargo check -p deno_npm_installer` Closes #25416 Closes denoland/divybot#364 --------- Co-authored-by: divybot <divybot@users.noreply.github.com> Co-authored-by: Divy Srivastava <me@littledivy.com>
1 parent 9935b92 commit 4acfa5d

11 files changed

Lines changed: 450 additions & 21 deletions

File tree

cli/npm.rs

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -417,6 +417,7 @@ impl LifecycleScriptsExecutor for DenoTaskLifeCycleScriptsExecutor {
417417
let layers = compute_lifecycle_script_layers(
418418
options.packages_with_scripts,
419419
options.snapshot,
420+
options.additional_packages,
420421
);
421422

422423
for layer in &layers {
@@ -547,6 +548,7 @@ impl DenoTaskLifeCycleScriptsExecutor {
547548
base_custom_commands.clone(),
548549
package,
549550
options.snapshot,
551+
options.additional_packages,
550552
)
551553
.await;
552554

@@ -727,6 +729,7 @@ impl DenoTaskLifeCycleScriptsExecutor {
727729
baseline: crate::task_runner::TaskCustomCommands,
728730
package: &NpmResolutionPackage,
729731
snapshot: &NpmResolutionSnapshot,
732+
additional_packages: &[&NpmResolutionPackage],
730733
) -> crate::task_runner::TaskCustomCommands {
731734
let sys = CliSys::default();
732735
let mut bin_entries = BinEntries::new(sys.with_paths_in_errors());
@@ -737,7 +740,12 @@ impl DenoTaskLifeCycleScriptsExecutor {
737740
baseline,
738741
snapshot,
739742
package.dependencies.iter().filter_map(|(name, id)| {
740-
let dep = snapshot.package_from_id(id).unwrap();
743+
let dep = snapshot.package_from_id(id).or_else(|| {
744+
additional_packages
745+
.iter()
746+
.find(|package| package.id == *id)
747+
.copied()
748+
})?;
741749
// Skip optional dependencies that don't match the current system
742750
if package.optional_dependencies.contains(name)
743751
&& !dep.system.matches_system(&self.system_info)

libs/npm_installer/hoisted.rs

Lines changed: 114 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -22,8 +22,11 @@ use std::sync::Arc;
2222

2323
use async_trait::async_trait;
2424
use deno_error::JsErrorBox;
25+
use deno_npm::NpmPackageExtraInfo;
2526
use deno_npm::NpmPackageId;
27+
use deno_npm::NpmPackageIdPeerDependencies;
2628
use deno_npm::NpmResolutionPackage;
29+
use deno_npm::NpmResolutionPackageSystemInfo;
2730
use deno_npm::NpmSystemInfo;
2831
use deno_npm::resolution::NpmResolutionSnapshot;
2932
use deno_npm_cache::NpmCache;
@@ -63,6 +66,7 @@ use crate::local::LocalNpmInstallSys;
6366
use crate::local::LocalNpmPackageInstallerOptions;
6467
use crate::local::SyncResolutionWithFsError;
6568
use crate::local::join_package_name;
69+
use crate::package_json::InstallWorkspacePkgDep;
6670
use crate::package_json::NpmInstallDepsProvider;
6771
use crate::process_state::NpmProcessState;
6872

@@ -347,7 +351,12 @@ impl<
347351
snapshot: &NpmResolutionSnapshot,
348352
) -> Result<(), SyncResolutionWithFsError> {
349353
let has_no_packages = snapshot.is_empty()
350-
&& self.npm_install_deps_provider.local_pkgs().is_empty();
354+
&& self.npm_install_deps_provider.local_pkgs().is_empty()
355+
&& !self
356+
.npm_install_deps_provider
357+
.workspace_pkgs()
358+
.iter()
359+
.any(|pkg| !pkg.scripts.is_empty());
351360
let deno_marker_dir = self.root_node_modules_path.join(".deno");
352361
if has_no_packages
353362
&& (!self.clean_on_install
@@ -398,6 +407,8 @@ impl<
398407
}
399408

400409
// 2. Clone all packages from cache into their hoisted positions
410+
let workspace_lifecycle_packages =
411+
self.resolve_workspace_lifecycle_packages(snapshot)?;
401412
let mut cache_futures = FuturesUnordered::new();
402413
let bin_entries = Rc::new(RefCell::new(BinEntries::new(sys)));
403414
let lifecycle_scripts = Rc::new(RefCell::new(LifecycleScripts::new(
@@ -538,6 +549,7 @@ impl<
538549
while let Some(result) = cache_futures.next().await {
539550
result?;
540551
}
552+
drop(cache_futures);
541553

542554
// 5. Set up bin entries
543555
{
@@ -582,6 +594,18 @@ impl<
582594
}
583595
}
584596

597+
for package in &workspace_lifecycle_packages {
598+
lifecycle_scripts.borrow_mut().add(
599+
&package.package,
600+
&NpmPackageExtraInfo {
601+
scripts: package.scripts.clone(),
602+
..Default::default()
603+
},
604+
Cow::Borrowed(&package.package_path),
605+
Vec::new(),
606+
);
607+
}
608+
585609
// Deprecation warnings
586610
{
587611
let packages_with_deprecation_warnings =
@@ -621,7 +645,7 @@ impl<
621645
}
622646

623647
// Lifecycle scripts
624-
let lifecycle_scripts = std::mem::replace(
648+
let lifecycle_scripts_to_run = std::mem::replace(
625649
&mut *lifecycle_scripts.borrow_mut(),
626650
LifecycleScripts::new(
627651
sys.as_ref(),
@@ -631,10 +655,16 @@ impl<
631655
},
632656
),
633657
);
634-
lifecycle_scripts.warn_not_run_scripts()?;
658+
drop(lifecycle_scripts);
659+
lifecycle_scripts_to_run.warn_not_run_scripts()?;
635660

636-
let packages_with_scripts = lifecycle_scripts.packages_with_scripts();
661+
let packages_with_scripts =
662+
lifecycle_scripts_to_run.packages_with_scripts();
637663
if !packages_with_scripts.is_empty() {
664+
let additional_packages = workspace_lifecycle_packages
665+
.iter()
666+
.map(|package| &package.package)
667+
.collect::<Vec<_>>();
638668
let process_state = NpmProcessState::new_local(
639669
snapshot.as_valid_serialized(),
640670
&self.root_node_modules_path,
@@ -651,6 +681,7 @@ impl<
651681
on_ran_pkg_scripts: &|_pkg| Ok(()),
652682
snapshot,
653683
system_packages: &package_partitions.packages,
684+
additional_packages: &additional_packages,
654685
packages_with_scripts,
655686
extra_info_provider: &extra_info_provider,
656687
})
@@ -664,6 +695,68 @@ impl<
664695
Ok(())
665696
}
666697

698+
fn resolve_workspace_lifecycle_packages(
699+
&self,
700+
snapshot: &NpmResolutionSnapshot,
701+
) -> Result<Vec<WorkspaceLifecyclePackage>, JsErrorBox> {
702+
let workspace_pkgs = self.npm_install_deps_provider.workspace_pkgs();
703+
let workspace_pkg_ids = workspace_pkgs
704+
.iter()
705+
.map(|pkg| {
706+
(
707+
pkg.nv.clone(),
708+
NpmPackageId {
709+
nv: pkg.nv.clone(),
710+
peer_dependencies: NpmPackageIdPeerDependencies::from([]),
711+
},
712+
)
713+
})
714+
.collect::<HashMap<_, _>>();
715+
let mut packages = Vec::with_capacity(workspace_pkgs.len());
716+
for workspace_pkg in workspace_pkgs {
717+
let mut dependencies = HashMap::with_capacity(workspace_pkg.deps.len());
718+
for dep in &workspace_pkg.deps {
719+
match dep {
720+
InstallWorkspacePkgDep::Remote { alias, req } => {
721+
if let Some(id) = resolve_remote_pkg_id(snapshot, req) {
722+
dependencies.insert(alias.clone(), id);
723+
}
724+
}
725+
InstallWorkspacePkgDep::Workspace { alias, nv } => {
726+
if let Some(id) = workspace_pkg_ids.get(nv) {
727+
dependencies.insert(alias.clone(), id.clone());
728+
}
729+
}
730+
}
731+
}
732+
let id = workspace_pkg_ids
733+
.get(&workspace_pkg.nv)
734+
.expect("workspace package id should exist")
735+
.clone();
736+
packages.push(WorkspaceLifecyclePackage {
737+
package: NpmResolutionPackage {
738+
id,
739+
copy_index: 0,
740+
system: NpmResolutionPackageSystemInfo::default(),
741+
dist: None,
742+
dependencies,
743+
optional_dependencies: Default::default(),
744+
optional_peer_dependencies: Default::default(),
745+
extra: Some(NpmPackageExtraInfo {
746+
scripts: workspace_pkg.scripts.clone(),
747+
..Default::default()
748+
}),
749+
is_deprecated: false,
750+
has_bin: false,
751+
has_scripts: !workspace_pkg.scripts.is_empty(),
752+
},
753+
package_path: workspace_pkg.target_dir.clone(),
754+
scripts: workspace_pkg.scripts.clone(),
755+
});
756+
}
757+
Ok(packages)
758+
}
759+
667760
#[allow(
668761
clippy::too_many_arguments,
669762
reason = "many parameters needed for package cloning"
@@ -773,6 +866,23 @@ impl<
773866
}
774867
}
775868

869+
struct WorkspaceLifecyclePackage {
870+
package: NpmResolutionPackage,
871+
package_path: PathBuf,
872+
scripts: HashMap<deno_semver::SmallStackString, String>,
873+
}
874+
875+
fn resolve_remote_pkg_id(
876+
snapshot: &NpmResolutionSnapshot,
877+
req: &deno_semver::package::PackageReq,
878+
) -> Option<NpmPackageId> {
879+
match snapshot.resolve_pkg_from_pkg_req(req) {
880+
Ok(pkg) => Some(pkg.id.clone()),
881+
Err(_) if req.version_req.tag().is_some() => None,
882+
Err(_) => snapshot.resolve_best_package_id(&req.name, &req.version_req),
883+
}
884+
}
885+
776886
fn cleanup_hoisted_packages(
777887
sys: &impl LocalNpmInstallSys,
778888
root_node_modules_path: &Path,

0 commit comments

Comments
 (0)