Skip to content

fix(node): Read module versions from the pnpm isolated layout - #25526

Open
JPeer264 wants to merge 1 commit into
developfrom
jp/node-modules-pnpm-isolated
Open

JPeer264 wants to merge 1 commit into
developfrom
jp/node-modules-pnpm-isolated

Conversation

@JPeer264

Copy link
Copy Markdown
Member

This fixes modulesIntegration for CJS apps that pnpm installs with its default isolated layout. The integration reads the version of each module in require.cache from its package.json, but only for packages directly in a node_modules folder of the app. pnpm keeps them in node_modules/.pnpm/<package id>/node_modules/<name>, so event.modules had only the version ranges from the app's package.json. Now it has the installed versions, and also the packages that the app does not declare, like with npm and Yarn.

The new suites/modules test writes a package into this layout, because the monorepo installs with the hoisted layout.

@JPeer264
JPeer264 added this pull request to stack #25528 October 19, 2026 13:44
`modulesIntegration` reads the version of each CJS module in
`require.cache` from its package.json in a node_modules folder of the
main module. The isolated pnpm layout keeps the package files in
`node_modules/.pnpm/<package id>/node_modules/<name>`, so the
integration found no version there. Events got only the version ranges
from the app's package.json, and no packages that the app does not
declare.

Read these folders like the node_modules folders of the main module.
The new test writes a package into this layout, because the monorepo
installs its packages with the hoisted layout.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@JPeer264
JPeer264 force-pushed the jp/node-modules-pnpm-isolated branch from aab2593 to 220ad81 Compare October 19, 2026 14:33
@JPeer264
JPeer264 marked this pull request as ready for review October 19, 2026 14:57
@JPeer264
JPeer264 requested a review from a team as a code owner October 19, 2026 14:57
@JPeer264
JPeer264 requested review from isaacs and mydea and removed request for a team October 19, 2026 14:57
@JPeer264 JPeer264 self-assigned this Oct 19, 2026
@JPeer264
JPeer264 requested review from andreiborza and s1gr1d and removed request for mydea October 19, 2026 14:57

@isaacs isaacs left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a surprising load-order issue, and we should perhaps add support for @scope/pkg while we're changing it.

return true;
}

const storeDir = dirname(dirname(dir));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pnpm/Bun store holds every version of every package, including transitive ones. Each store entry now counts as a "main" package, so infos[info.name] = info.version takes whichever version require.cache lists last. I reproduced this with a fake pnpm layout: the app depends on debug@4.0.0, and lib depends on debug@2.0.0.

require('debug'); require('lib');  -> { debug: '2.0.0', lib: '1.0.0' }
require('lib'); require('debug');  -> { lib: '1.0.0', debug: '4.0.0' }

Before this PR, pnpm users got the range from package.json (^4...), which was at least the right major. Now they can get a wrong version, and which one changes with load order. With npm, nested copies (node_modules/a/node_modules/b) are not in mainPaths, so the top-level (app-facing) copy always wins. The PR body says the result is "like with npm and Yarn", but here it differs.

Suggestion: when a name has more than one store version, prefer the one that <main node_modules>/<name> resolves to. For example, collect store hits per name, and on conflict compare realpathSync(join(mainModulesDir, name)) with the package folder. Or, simpler: never let a store entry overwrite a name that already came from a store entry that the top-level symlink points to.

* `<that node_modules>/<.pnpm or .bun>/<package id>/node_modules`, where the isolated layouts of pnpm and Bun keep
* each package.
*/
function isMainModulesDir(dir: string, mainPaths: string[]): boolean {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For .../node_modules/@scope/name/index.js, the walk checks .../node_modules/@scope (not a modules folder), then .../node_modules with orig = .../@scope, finds no @scope/package.json, and moves on. So no scoped package is ever read from require.cache, in any layout. I confirmed this with real installs: @isaacs/cliui is missing from the npm, pnpm, and Bun results (it only shows up when the cwd package.json lists it, as a range). This is not a regression, but the PR's goal is to report installed versions for pnpm users, and many of their deps are scoped (@sentry/*, @opentelemetry/*, @aws-sdk/*, ...).

Suggestion (could be a follow-up, since it's a pre-existing issue, but I'd fix it now, why not): treat a scope folder as part of the modules folder. I tested this one-line change on the built output, and it fixes scoped packages in the hoisted, pnpm, and Bun layouts:

function isMainModulesDir(dir: string, mainPaths: string[]): boolean {
  if (basename(dir).startsWith('@')) {
    return isMainModulesDir(dirname(dir), mainPaths);
  }
  // ...
}

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants