Conversation
The isolated global packages refactor introduced a custom shortcut for `pnpm -g ls` that always returned plain text, breaking tools like npm-check-updates that parse the JSON output. Aggregate global packages into a synthesized dependency hierarchy and dispatch to the regular renderers so `--json`/`--parseable` work like in pnpm 10. Closes #11440
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughGlobal ChangesGlobal List Command Output & Depth Handling
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (user)
participant Handler as Listing Handler
participant Finder as findGlobalInstallDirs
participant Lister as listGlobalPackages
participant FS as Filesystem
participant Renderer as renderJson/renderParseable/renderTree
CLI->>Handler: pnpm -g ls [--json|--parseable|--depth]
Handler->>Finder: if depth>0, discover install dirs (globalPkgDir, params)
Finder->>FS: read global package metadata and installDir values
Finder-->>Handler: 0, 1, or multiple install dirs
Handler->>Renderer: if 0/1 use render path (single dir -> render full tree)
Handler->>CLI: error GLOBAL_LS_DEPTH_NOT_SUPPORTED when multiple ambiguous dirs
Handler->>Lister: if depth===0 call listGlobalPackages(globalPkgDir, params, opts)
Lister->>FS: read packages, build DependencyNode[]
Lister->>Renderer: render based on reportAs (json/parseable/tree)
Renderer->>CLI: emit formatted output
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@global/commands/src/listGlobalPackages.ts`:
- Around line 45-48: The early return for the empty dependencies case returns an
object missing the dependencies field; change it so the returned JSON object
matches the normal renderer schema by including an empty dependencies array. In
the branch where reportAs === 'json' and dependencies.length === 0 (refer to the
dependencies variable, reportAs check, and globalPkgDir), return an object with
path: globalPkgDir, private: true and dependencies: [] serialized with
JSON.stringify so downstream consumers can safely read result[0].dependencies.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: acbc8928-808f-4d13-b7ba-b45638d51241
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
.changeset/global-ls-json-parseable.mddeps/inspection/commands/src/listing/list.tsglobal/commands/package.jsonglobal/commands/src/listGlobalPackages.tsglobal/commands/test/listGlobalPackages.test.tsglobal/commands/tsconfig.jsonpnpm/test/install/global.ts
…stalls Each global package lives in its own install dir with its own lockfile, so the same transitive can resolve to different versions across installs. Merging them would be incoherent. When the request narrows to a single install group, fall through to the regular list flow on that install dir and produce the full dependency tree.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deps/inspection/commands/src/listing/list.ts`:
- Around line 119-137: When depth>0, don't let the package-name filtering in
params cause findGlobalInstallDirs(...) to return no installs; call
findGlobalInstallDirs in a way that ignores the package filter (e.g. pass params
stripped of search filters or call the overload that only uses
opts.globalPkgDir) so that if installDirs.length === 1 we still take the
single-install fast path and call render([installDirs[0]], params, {...}) as
before; this lets listForPackages()/render perform the actual dependency
filtering instead of falling back to listGlobalPackages(). Update the call site
that currently uses findGlobalInstallDirs(opts.globalPkgDir, params) to use an
unfiltered params variant (while keeping the original params when invoking
render) and add/update a test covering `@pnpm.e2e/dep-of-pkg-with-1-dep` for pnpm
ls -g <transitive-dep> --depth=1.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: aa6b16be-384b-4d00-9b0d-8674cf5e03f9
📒 Files selected for processing (5)
.changeset/global-ls-json-parseable.mddeps/inspection/commands/src/listing/list.tsglobal/commands/src/index.tsglobal/commands/src/listGlobalPackages.tspnpm/test/install/global.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/global-ls-json-parseable.md
…install When `pnpm -g ls <pkg> --depth>0` resolves to a single install dir, dropping the params before delegating to render avoids activating the listForPackages search semantics (which would prune the matched package's children) and shows the full tree instead.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pnpm/test/install/global.ts`:
- Around line 490-504: The test 'global ls --parseable outputs paths' currently
parses stdout using split('\n'), which leaves trailing '\r' on Windows and
breaks the endsWith(...) assertion; change the parsing after execPnpmSync (the
stdout returned by execPnpmSync in this test) to split using a CRLF-safe regex
and normalize each line (e.g., split with /\r?\n/, then map each line through
trim() and filter out empty strings) so the subsequent expect(lines.some(...))
check is reliable across platforms.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 54d01ee3-e7f9-4d04-a55f-a3dfc205b87b
📒 Files selected for processing (2)
deps/inspection/commands/src/listing/list.tspnpm/test/install/global.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- deps/inspection/commands/src/listing/list.ts
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated 3 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Single global install fast-path: delegate to render with params unchanged so listForPackages can match transitive deps too. Previously, `pnpm -g ls <transitive> --depth=1` wrongly reported "no matching" because findGlobalInstallDirs only knows about top-level aliases. - Multi-install path: same narrow-by-top-level-alias logic as before; drop params after narrowing so the install's full tree shows. - Drop direct dep on @pnpm/deps.inspection.tree-builder: DependencyNode is re-exported by @pnpm/deps.inspection.list. - Use CRLF-tolerant split in the parseable-output e2e test.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
deps/inspection/commands/src/listing/list.ts (1)
118-157: 🏗️ Heavy liftCache the global scan before the branch.
findGlobalInstallDirs()scans the filesystem internally, so this path now walks the same global directories twice before it decides whether to render or throw. Reusing one scan snapshot would cut IO and avoid racing against concurrent installs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deps/inspection/commands/src/listing/list.ts` around lines 118 - 157, Call findGlobalInstallDirs(opts.globalPkgDir, []) once and reuse that result instead of invoking it twice; compute allInstallDirs from that cached snapshot, then derive matchingInstallDirs by filtering that cached list against params (or by calling a cheap in-memory matcher using params) so the filesystem is scanned only once; update references to allInstallDirs and matchingInstallDirs in the branch around render(...) and the PnpmError throw to use the cached data.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@deps/inspection/commands/src/listing/list.ts`:
- Around line 118-157: Call findGlobalInstallDirs(opts.globalPkgDir, []) once
and reuse that result instead of invoking it twice; compute allInstallDirs from
that cached snapshot, then derive matchingInstallDirs by filtering that cached
list against params (or by calling a cheap in-memory matcher using params) so
the filesystem is scanned only once; update references to allInstallDirs and
matchingInstallDirs in the branch around render(...) and the PnpmError throw to
use the cached data.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 173f6d4c-c893-4ea0-a30e-6f57805ba211
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
deps/inspection/commands/src/listing/list.tsglobal/commands/package.jsonglobal/commands/src/listGlobalPackages.tsglobal/commands/tsconfig.jsonpnpm/test/install/global.ts
✅ Files skipped from review due to trivial changes (1)
- global/commands/package.json
🚧 Files skipped from review as they are similar to previous changes (2)
- global/commands/tsconfig.json
- pnpm/test/install/global.ts
Restores `--json` / `--parseable` / `--long` support on `pnpm -g ls` and tightens `--depth>0` semantics around isolated global installs. Closes #11440. - **`--json` / `--parseable` (the regression):** aggregate global packages from all isolated install dirs into a single synthesized `PackageDependencyHierarchy` and dispatch to the existing `renderJson` / `renderParseable` / `renderTree`. Output shape matches pnpm 10 (`result[0].dependencies[name].version`), so tools like `npm-check-updates` work again. - **`--depth>0`:** the v11 architecture installs each global package into its own isolated dir with its own lockfile, so merging transitive trees across installs would be incoherent. New behavior: - One global install dir total → fast-path delegate to the regular `list` flow with `params` unchanged, so `listForPackages` can match top-level *or* transitive packages. - Multiple installs, params narrow to one install dir (top-level alias match) → drop the params and render that install dir's full tree. - Multiple installs, params don't narrow → throw `ERR_PNPM_GLOBAL_LS_DEPTH_NOT_SUPPORTED` with a message asking the user to filter to a single global package or omit `--depth`. The regression was introduced by the isolated global packages refactor (#10697), which added a custom `listGlobalPackages` shortcut that always returned plain text and ignored format flags.
Summary
Restores
--json/--parseable/--longsupport onpnpm -g lsand tightens--depth>0semantics around isolated global installs. Closes #11440.--json/--parseable(the regression): aggregate global packages from all isolated install dirs into a single synthesizedPackageDependencyHierarchyand dispatch to the existingrenderJson/renderParseable/renderTree. Output shape matches pnpm 10 (result[0].dependencies[name].version), so tools likenpm-check-updateswork again.--depth>0: the v11 architecture installs each global package into its own isolated dir with its own lockfile, so merging transitive trees across installs would be incoherent. New behavior:listflow withparamsunchanged, solistForPackagescan match top-level or transitive packages.ERR_PNPM_GLOBAL_LS_DEPTH_NOT_SUPPORTEDwith a message asking the user to filter to a single global package or omit--depth.The regression was introduced by the isolated global packages refactor (#10697), which added a custom
listGlobalPackagesshortcut that always returned plain text and ignored format flags.Test plan
global/commands/test/listGlobalPackages.test.tscover json / parseable / tree / empty / param filtering for the depth=0 path.pnpm/test/install/global.ts:pnpm -g ls --jsonreturns valid JSONpnpm -g ls --parseablereturns paths (CRLF-tolerant)pnpm -g ls --depth=1errors with multiple isolated installspnpm -g ls --depth=1shows the full tree when there is one global installpnpm -g ls <transitive> --depth=1matches transitive deps vialistForPackagessearch semantics (single-install fast path)pnpm -g ls <pkg> --depth=1narrows to the install dir containing<pkg>and shows its treepnpm --filter @pnpm/global.commands run compilepasses (lint included).pnpm --filter @pnpm/deps.inspection.commands run compilepasses.