mirror of
https://github.com/pnpm/pnpm.git
synced 2026-07-19 04:02:32 -04:00
fix: render NDJSON output cleanly through @pnpm/default-reporter (#403)
* fix(cli): canonicalize `--dir` so reporter `prefix` matches subprocess `process.cwd()` `pacquet --reporter=ndjson` piped into `@pnpm/cli.default-reporter` emitted every progress / stats line with a `. | ` path prefix because the bunyan-envelope `prefix` field was the literal `"."` (derived from the manifest's parent of `./package.json`). The reporter compares each event's `prefix` to its own `cwd` (an absolute, canonicalized path from `process.cwd()`), and the mismatch routed every log through `zoomOut`. Canonicalize `--dir` once at the top of `CliArgs::run`, mirroring pnpm's `config/reader/src/index.ts:270` where `cwd = fs.realpathSync(betterPathResolve(cliOptions.dir ...))` becomes `pnpmConfig.dir` and is threaded into every event's `prefix` as `lockfileDir`. Ports the same realpath behavior to pacquet so a default-reporter subprocess and pacquet agree on the project root. Side effect: `pacquet init` now derives the package `name` from the working-directory basename (matching pnpm) instead of leaving it as `""` — the `.` parent had no `file_name`. Snapshot updated. * Apply suggestions from code review Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * fix(tarball): emit `pnpm:fetching-progress started` with one-indexed `attempt` The default reporter's `reportBigTarballsProgress` filters started events with `log.attempt === 1` so a transient retry doesn't reset the "Downloading <pkg>: …/<size>" progress line. Pacquet's loop counter is zero-indexed and was being passed straight through, so every started event was emitted with `attempt: 0` and the reporter silently dropped the entire channel — no big-tarball progress lines ever rendered. Pnpm's underlying `node-retry` `op.attempt(cb)` callback hands `cb` a 1-indexed counter, which `packageRequester` forwards verbatim into the log. Match that wire shape by emitting `attempt + 1` at the fetching-progress site, mirroring the same `+ 1` adjustment already present at the neighbouring `pnpm:request-retry` emit. Updates the recording-fake test to assert `[1, 2]` (one-indexed) and fixes a stray `attempt: 0` in the reporter serialization test. * test(cli): add NDJSON `prefix` regression test, refine review nits Addresses three review comments on #403: * Add an integration test (`install_emits_canonical_prefix_in_ndjson_events`) that runs `pacquet --reporter=ndjson install` and asserts every event's `prefix` is the canonicalized workspace path — never `"."`. Pins the user-visible regression: the original failure mode was the default-reporter subprocess prepending `<prefix> | ` to every progress / stats line because the emitted `"."` never matched its own `process.cwd()`. * Reword the canonicalize-failure context to reference the `--dir` argument explicitly so a stat / permission failure points the user at the flag they passed. * Restore the `assertion_line` field on `init__should_create_package_json.snap` so its metadata matches the surrounding insta snapshots. --------- Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
This commit is contained in:
@@ -29,6 +29,7 @@ pacquet-diagnostics = { workspace = true }
|
||||
|
||||
clap = { workspace = true }
|
||||
derive_more = { workspace = true }
|
||||
dunce = { workspace = true }
|
||||
home = { workspace = true }
|
||||
miette = { workspace = true }
|
||||
pipe-trait = { workspace = true }
|
||||
@@ -40,7 +41,6 @@ pacquet-store-dir = { workspace = true }
|
||||
pacquet-testing-utils = { workspace = true }
|
||||
|
||||
assert_cmd = { workspace = true }
|
||||
dunce = { workspace = true }
|
||||
command-extra = { workspace = true }
|
||||
insta = { workspace = true }
|
||||
pretty_assertions = { workspace = true }
|
||||
|
||||
@@ -7,7 +7,7 @@ use crate::State;
|
||||
use add::AddArgs;
|
||||
use clap::{Parser, Subcommand, ValueEnum};
|
||||
use install::InstallArgs;
|
||||
use miette::Context;
|
||||
use miette::{Context, IntoDiagnostic};
|
||||
use pacquet_executor::execute_shell;
|
||||
use pacquet_npmrc::Npmrc;
|
||||
use pacquet_package_manifest::PackageManifest;
|
||||
@@ -82,6 +82,19 @@ impl CliArgs {
|
||||
/// Execute the command
|
||||
pub async fn run(self) -> miette::Result<()> {
|
||||
let CliArgs { command, dir, reporter } = self;
|
||||
// Canonicalize `--dir` so the bunyan-envelope `prefix` emitted by
|
||||
// the reporter is the same absolute, symlink-resolved path that
|
||||
// `@pnpm/cli.default-reporter` derives via `process.cwd()`. Without
|
||||
// this, a default `--dir=.` leaves `prefix` as `"."`, the reporter
|
||||
// never matches it against its `cwd`, and every progress / stats
|
||||
// line gets a redundant `.` path prefix prepended. Mirrors pnpm's
|
||||
// <https://github.com/pnpm/pnpm/blob/8eb1be4988/config/reader/src/index.ts#L270>
|
||||
// `cwd = fs.realpathSync(betterPathResolve(cliOptions.dir ...))`,
|
||||
// later assigned to `pnpmConfig.dir` (used as the install
|
||||
// `lockfileDir`, threaded into every event's `prefix`).
|
||||
let dir = dunce::canonicalize(&dir)
|
||||
.into_diagnostic()
|
||||
.wrap_err_with(|| format!("canonicalizing the `--dir` argument: {}", dir.display()))?;
|
||||
let manifest_path = || dir.join("package.json");
|
||||
let npmrc = || Npmrc::current(env::current_dir, home::home_dir, Default::default).leak();
|
||||
// `require_lockfile` is the "this subcommand cannot run without a
|
||||
|
||||
@@ -170,6 +170,62 @@ fn frozen_lockfile_should_be_able_to_handle_big_lockfile() {
|
||||
drop((root, mock_instance)); // cleanup
|
||||
}
|
||||
|
||||
/// Regression test for the NDJSON `prefix` field. `--reporter=ndjson`
|
||||
/// must emit each bunyan envelope with the canonicalized install root
|
||||
/// — not the relative `"."` that `dir.join("package.json").parent()`
|
||||
/// produced when `--dir` defaulted to `.`. The downstream consumer
|
||||
/// (`@pnpm/cli.default-reporter` running in a separate process) compares
|
||||
/// every event's `prefix` to its own `process.cwd()` and prepends a
|
||||
/// redundant `<prefix> | ` adornment whenever they disagree, so a `"."`
|
||||
/// prefix made every progress / stats line render with `. | `.
|
||||
#[test]
|
||||
fn install_emits_canonical_prefix_in_ndjson_events() {
|
||||
let CommandTempCwd { pacquet, root, workspace, npmrc_info, .. } =
|
||||
CommandTempCwd::init().add_mocked_registry();
|
||||
let AddMockedRegistry { mock_instance, .. } = npmrc_info;
|
||||
|
||||
eprintln!("Creating package.json...");
|
||||
let manifest_path = workspace.join("package.json");
|
||||
let package_json = serde_json::json!({
|
||||
"dependencies": {
|
||||
"@pnpm.e2e/hello-world-js-bin-parent": "1.0.0",
|
||||
},
|
||||
});
|
||||
fs::write(&manifest_path, package_json.to_string()).expect("write to package.json");
|
||||
|
||||
eprintln!("Executing command with --reporter=ndjson...");
|
||||
let output =
|
||||
pacquet.with_args(["--reporter=ndjson", "install"]).output().expect("run pacquet install");
|
||||
assert!(
|
||||
output.status.success(),
|
||||
"pacquet install exited non-zero: stderr={}",
|
||||
String::from_utf8_lossy(&output.stderr),
|
||||
);
|
||||
|
||||
eprintln!("Collecting `prefix` values from NDJSON stderr...");
|
||||
let stderr = String::from_utf8(output.stderr).expect("stderr is utf-8");
|
||||
let prefixes: Vec<String> = stderr
|
||||
.lines()
|
||||
.filter_map(|line| serde_json::from_str::<serde_json::Value>(line).ok())
|
||||
.filter_map(|val| val.get("prefix").and_then(|p| p.as_str()).map(str::to_owned))
|
||||
.collect();
|
||||
assert!(
|
||||
!prefixes.is_empty(),
|
||||
"expected at least one event with a `prefix` field; stderr was:\n{stderr}",
|
||||
);
|
||||
|
||||
let expected = dunce::canonicalize(&workspace).expect("canonicalize workspace");
|
||||
let expected = expected.to_str().expect("workspace path is UTF-8");
|
||||
for prefix in &prefixes {
|
||||
assert_eq!(
|
||||
prefix, expected,
|
||||
"every event's prefix must be the canonicalized install root, not relative",
|
||||
);
|
||||
}
|
||||
|
||||
drop((root, mock_instance)); // cleanup
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn should_install_circular_dependencies() {
|
||||
let CommandTempCwd { pacquet, root, workspace, npmrc_info, .. } =
|
||||
|
||||
@@ -1,10 +1,10 @@
|
||||
---
|
||||
source: crates/cli/tests/init.rs
|
||||
assertion_line: 31
|
||||
assertion_line: 20
|
||||
expression: package_json_content
|
||||
---
|
||||
{
|
||||
"name": "",
|
||||
"name": "workspace",
|
||||
"version": "1.0.0",
|
||||
"description": "",
|
||||
"main": "index.js",
|
||||
|
||||
@@ -232,7 +232,7 @@ fn fetching_progress_event_matches_pnpm_wire_shape() {
|
||||
let event = LogEvent::FetchingProgress(FetchingProgressLog {
|
||||
level: LogLevel::Debug,
|
||||
message: FetchingProgressMessage::Started {
|
||||
attempt: 0,
|
||||
attempt: 1,
|
||||
package_id: "react@18.0.0".to_string(),
|
||||
size: None,
|
||||
},
|
||||
|
||||
@@ -838,12 +838,21 @@ async fn fetch_and_extract_once<R: Reporter>(
|
||||
// reporter checks `size != null` before rendering a percent
|
||||
// gauge, so this admits "we don't know yet" only when we truly
|
||||
// don't know.
|
||||
//
|
||||
// `attempt` is one-indexed (the in-flight attempt) to match
|
||||
// pnpm's wire shape — `node-retry`'s `op.attempt(cb)` callback
|
||||
// hands `cb` a 1-indexed counter, which `packageRequester`
|
||||
// forwards verbatim into the `attempt` field. Pacquet's loop
|
||||
// counter is zero-indexed, so emit `attempt + 1`. The default
|
||||
// reporter's `reportBigTarballsProgress` filters on
|
||||
// `log.attempt === 1` (so retries don't reset the progress
|
||||
// line), so a zero would silence every "Downloading …" line.
|
||||
let send_result = client.get(package_url).send().await;
|
||||
let size = send_result.as_ref().ok().and_then(|r| r.content_length());
|
||||
R::emit(&LogEvent::FetchingProgress(FetchingProgressLog {
|
||||
level: LogLevel::Debug,
|
||||
message: FetchingProgressMessage::Started {
|
||||
attempt,
|
||||
attempt: attempt + 1,
|
||||
package_id: package_id.to_owned(),
|
||||
size,
|
||||
},
|
||||
|
||||
@@ -1548,8 +1548,10 @@ async fn run_with_mem_cache_recovers_from_owning_fetch_error() {
|
||||
/// tarball pipeline:
|
||||
///
|
||||
/// * `pnpm:fetching-progress started` once per *attempt* — so a 503 +
|
||||
/// 200 retry pattern emits twice with `attempt = 0` then
|
||||
/// `attempt = 1`. `size` carries the response's `Content-Length`
|
||||
/// 200 retry pattern emits twice with `attempt = 1` then
|
||||
/// `attempt = 2` (one-indexed, matching pnpm's wire shape — the
|
||||
/// default reporter's `reportBigTarballsProgress` filters on
|
||||
/// `attempt === 1`). `size` carries the response's `Content-Length`
|
||||
/// (mockito sends one for `with_body`).
|
||||
/// * `pnpm:fetching-progress in_progress` is throttled to ~200ms; the
|
||||
/// tiny FASTIFY tarball used here downloads in well under that, so
|
||||
@@ -1621,7 +1623,7 @@ async fn fetching_progress_and_fetched_events_fire_during_download() {
|
||||
})
|
||||
.collect();
|
||||
let attempts: Vec<u32> = started.iter().map(|(a, _)| *a).collect();
|
||||
assert_eq!(attempts, vec![0, 1], "started must fire once per attempt; got {captured:?}");
|
||||
assert_eq!(attempts, vec![1, 2], "started must fire once per attempt; got {captured:?}");
|
||||
// Both attempts have a response head (mockito sends Content-Length
|
||||
// for `with_body(...)` and `with_status(503)` likewise), so both
|
||||
// `started` events must carry a populated `size`. Pinning this
|
||||
|
||||
Reference in New Issue
Block a user