fix(pm): run install lifecycle on workspace packages + unify lifecycle primitive#2839
fix(pm): run install lifecycle on workspace packages + unify lifecycle primitive#2839
Conversation
ut install and ut rebuild skipped lifecycle install hooks (prepare, postinstall, etc.) on workspace source packages, so a workspace consumer that imports another workspace's build output broke until the upstream package was built manually. This diverges from npm 7+, pnpm, yarn, and tnpm. Walk workspace packages in topological order after node_modules is linked and run the same PROJECT_INSTALL_HOOKS set the root already uses, before the root's own hooks. Closes #2833. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request introduces the process_workspace_install_hooks method to PackageService, enabling the execution of workspace lifecycle hooks in topological order. This ensures that dependencies are built before their consumers, specifically addressing issues where root project hooks depend on workspace artifacts. The RebuildService has been updated to incorporate these workspace hooks. Feedback suggests optimizing performance by parallelizing the execution of packages within the same topological layer.
| for layer in layers { | ||
| for name in layer { |
There was a problem hiding this comment.
The current implementation executes workspace hooks sequentially across all packages, even for those within the same topological layer. Since packages in the same layer are independent, they can be executed concurrently to improve performance in large monorepos. Consider parallelizing the execution of packages within each layer, similar to how run_in_layers handles it, while ensuring output remains readable (e.g., by capturing and grouping output).
Adds workspace-prepare fixture and three e2e cases that exercise the issue #2833 fix end-to-end: - ut install must run lib-a's prepare before lib-b's prepare so the consumer can see the producer's built artifact - ut rebuild must re-run workspace prepare/postinstall hooks - --ignore-scripts must skip workspace hooks (no surprise side effects) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Arborist test fixtures (e.g. workspaces-need-update) ship workspace package.json files without a `name` field. process_workspace_install_hooks called PackageInfo::load(path), which bailed with "Failed to get package name from package.json" and aborted the whole install. Mirror npm's `@npmcli/name-from-folder`: when a workspace package.json omits `name`, derive it from the folder layout (`@scope/foo` if the parent dir begins with `@`). Pipe the derived name through the topology and into PackageInfo via load_with_name_fallback. - ruborist::resolver::workspace: name_from_folder + bail on undriveable - ruborist::model::graph: PackageNode::*_from_package_json_with_name - pm::helper::tree_builder: pass derived name into PackageNode ctors - pm::model::package: from_package_json_with_name_fallback shared by load_with_name_fallback and from_package_json - e2e/pm/workspace-anonymous: integration smoke for the anonymous case Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Move the npm `name-from-folder` fallback into `find_workspaces` itself by writing the derived name back into `WorkspacePackage.package_json.name`, so every downstream consumer sees a non-empty `pkg.name` without needing parallel `_with_name` constructors. - ruborist::resolver::workspace: write fallback into the in-memory PackageJson - pm::service::package: drop `load_with_name_fallback`, reuse the normalized PackageJson from `find_workspaces` in `process_workspace_install_hooks` - pm::model::package: revert `load_with_name_fallback` / `from_package_json_with_name_fallback` - pm::helper::tree_builder: revert `_with_name` constructor calls - ruborist::model::graph: revert `_with_name` constructors Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collapse the duplicate `pre<event>` / `<event>` / `post<event>` chain implementations behind a single `ScriptService::run_lifecycle(package, event, args, sink, main_required)` primitive. The npm-event granularity (install / prepublish / prepare) replaces the per-hook 7-element loop that workspace + project install paths used to walk. - script.rs: add `LifecycleSink` (Stream / Capture with caller-owned buffers) and `run_lifecycle`. `run_script` and `run_script_captured` shrink to thin wrappers selecting sink + main_required. - service/package.rs: `process_workspace_install_hooks` / `process_project_hooks` now run 3 npm events through `run_lifecycle` via a shared `run_install_lifecycle` helper. Drops the 7-hook for-loop. - model/package.rs: remove `LifecycleHook::PROJECT_INSTALL_HOOKS` constant — no remaining users. Behaviour preserved: e2e workspace-prepare topological ordering, anonymous-workspace name-from-folder fallback, --ignore-scripts skip, ut run --workspaces layered output, --if-present skip, missing-script Fail-mode error message all verified against the release binary. Dependency-side `execute_script_queue` (per-hook slicing + join_all) and the publish/pack lifecycles keep using `execute_script` since they address single hooks by enum, not npm events. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Quality/efficiency cleanups from the simplify pass: - Replace `main_required: bool` with `MissingScript` enum on `run_lifecycle` — bool-arg anti-pattern; the enum already exists for the same Fail/Skip distinction at the run path. - Drop `(PathBuf, PackageInfo)` tuple in `process_workspace_install_hooks` — `PathBuf` was unread (consumer reads `package.path` from PackageInfo). - Convert the layers/map invariant `expect` to `with_context` — the invariant spans two service modules; a real error path is safer. - Trim WHAT comments per CLAUDE.md (run_lifecycle docs, NPM_INSTALL_EVENTS, process_workspace_install_hooks header, LifecycleSink variants); keep only WHY (e.g. capture buffer survival on partial failure, #2833 link). - Inline the `no_args` temp at the steps array. Behaviour preserved: cargo test -p utoo-pm --test-threads=1 (249 pass), clippy clean, e2e workspace-prepare topological order + run --workspaces + missing-script Fail error all match prior output. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
ut install/ut rebuildskipped lifecycle install hooks (prepare,postinstall, etc.) on workspace source packages, so a workspace consumer that imported another workspace's build output broke until the upstream package was built manually. This diverged from npm 7+, pnpm, yarn, and tnpm.After
node_modulesis linked, walk workspace packages in topological order and run npm's install lifecycle (install/prepublish/prepareevent chains) on each, before the root's own hooks — the root may import build artifacts produced by a workspace'sprepare.While here, fold two related cleanups in (small enough to ship together):
@npmcli/name-from-folderfallback) at discovery time so downstream consumers see a non-emptypkg.namewithout parallel_with_nameconstructors.pre<event>/<event>/post<event>chains into a singleScriptService::run_lifecycleprimitive used by bothrunand the install path.Closes #2833.
Behaviour changes (the fix)
PackageService::process_workspace_install_hooksresolves the workspace topology and runs each workspace's npm install lifecycle in order, gated onScriptPolicy::Runso--ignore-scriptssemantics are preserved.RebuildService::rebuildinvokes the workspace lifecycle walk beforeprocess_project_hooks.namefield — e.g. arborist'sworkspaces-need-updatefixture) get the npm folder-derived fallback insidefind_workspaces, so all downstream code sees a non-emptypkg.name.Refactors folded in
Anonymous workspace name normalization (
crates/ruborist/src/resolver/workspace.rs)The
name_from_folderfallback now writes back into the in-memoryWorkspacePackage.package_json.name. Removes the parallel_with_nameconstructors that an earlier draft sprinkled acrosstree_builder,model::package, andruborist::model::graph— those files are untouched now.ScriptService::run_lifecycleprimitive (crates/pm/src/service/script.rs)One function expresses "run
pre<event>/<event>/post<event>for a single package" parameterised by:LifecycleSink::Stream { workspace_label }— terminal streaming (used byrun_scriptand the install path).LifecycleSink::Capture { workspace_label, header, body }— caller-owned buffers; on partial failure the caller still has the bytes captured up to the failing step (used byrun_script_captured/run_in_layers).main_required: bool— whether a missing main script is an error (truefornpm run <name>,falsefor npm install events where any of the three may exist alone).Callers shrink to thin wrappers:
run_scriptrun_lifecycle(Stream, main_required: true)run_script_capturedrun_lifecycle(Capture, main_required: false)+ friendly missing-script CLI hintprocess_workspace_install_hooksexecute_scriptrun_lifecyclevia sharedrun_install_lifecyclehelperprocess_project_hooksDrops the
LifecycleHook::PROJECT_INSTALL_HOOKSconstant (no remaining users). Dependency-sideexecute_script_queue(per-hook slicing +join_all) and the publish/pack lifecycles keep usingexecute_scriptsince they address single hooks by enum, not npm events.Changes
crates/ruborist/src/resolver/workspace.rs— writename_from_folderfallback back intoWorkspacePackage.package_json.name; addtest_name_from_foldercovering plain / @scope / root paths.crates/pm/src/service/script.rs— addLifecycleSink+run_lifecycle; rewriterun_scriptandrun_script_capturedas thin wrappers.crates/pm/src/service/package.rs—process_workspace_install_hooksresolves topology + loads workspaces with normalized names;process_project_hooksandprocess_workspace_install_hooksroute through newrun_install_lifecyclehelper usingNPM_INSTALL_EVENTS = ["install", "prepublish", "prepare"].crates/pm/src/service/rebuild.rs— invoke workspace lifecycle walk before project hooks, gated onScriptPolicy::Run.crates/pm/src/model/package.rs— dropLifecycleHook::PROJECT_INSTALL_HOOKS.Test plan
crates/pm/src/service/package.rs:test_process_workspace_install_hooks_topological_order— B depends on A; A'sprepareproduceslib/marker; B'sprepareasserts the marker exists.test_process_workspace_install_hooks_no_workspaces— single-package project is a no-op success.test_process_workspace_install_hooks_propagates_failure— a failing workspacepreparesurfaces as an install error.test_process_workspace_install_hooks_anonymous_workspaces— both anonymous siblings run their hooks via the folder-name fallback.crates/ruborist/src/resolver/workspace.rs—test_name_from_folderfor plain /@scope/foo/ root paths.e2e/utoo-pm.sh):--ignore-scripts: hooks skipped, bin link still runstarget/release/utoo:ut run build --workspaces(layered output unchanged)ut run build --workspace lib-b(single-workspace layered path)ut run test --workspaces --if-present(silent skip)ut run nonexistent --workspaces(Fail-mode missing-script error preserved)cargo test -p utoo-pm -- --test-threads=1— 249 passed (parallel runs hit a pre-existing cwd race in unrelated tests, not caused by this PR)cargo fmt --checkcleancargo clippy --all-targets --no-deps -- -D warningsclean🤖 Generated with Claude Code