deps: migrate pnpm internals to v12#10293
Open
zkochan wants to merge 12 commits into
Open
Conversation
zkochan
force-pushed
the
pnpm11
branch
2 times, most recently
from
3d42363 to
37f4352
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR migrates Bit’s internal pnpm integration to pnpm v11-era packages (1100.x), updating dependency names and adapting Bit’s pnpm/yarn package-manager glue code to pnpm’s breaking API and module-format (ESM) changes.
Changes:
- Renames and bumps
@pnpm/*dependencies inworkspace.jsoncto pnpm v11 package naming and versions. - Updates pnpm/yarn integration code for pnpm v11 APIs (config/auth shape, resolver creation, store controller creation, peer issues options, project graph/sorting).
- Adds CJS shims + adjusts Babel config to safely load ESM-only pnpm modules in test/build environments without breaking mocha/Babel require chains.
Reviewed changes
Copilot reviewed 25 out of 26 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| workspace.jsonc | Renames pnpm internal deps to v11 package names and bumps versions. |
| scopes/workspace/workspace/workspace-aspects-loader.ts | Removes unreachable code in error handling path. |
| scopes/typescript/typescript/schema-extractor-context.ts | Removes unreachable return after throwing. |
| scopes/scope/objects/models/dependencies-graph.ts | Updates pnpm dep-path import to new package name. |
| scopes/pkg/pkg/packer.ts | Switches pnpm pack command loading to ESM-safe dynamic loader shim. |
| scopes/pkg/pkg/load-pnpm-pack.cjs | New CJS shim to import() ESM-only pnpm releasing commands for packing. |
| scopes/harmony/aspect/babel/babel-config.ts | Adjusts preset-env to avoid transforming dynamic import(). |
| scopes/dependencies/yarn/yarn.package-manager.ts | Updates pnpm overrides parser import to v11 package. |
| scopes/dependencies/pnpm/read-config.ts | Switches pnpm config reader import to v11 package. |
| scopes/dependencies/pnpm/pnpm.package-manager.ts | Adapts network config reading + lockfile graph converter init + updated pnpm module imports. |
| scopes/dependencies/pnpm/pnpm-error-to-bit-error.spec.ts | Avoids importing ESM-only @pnpm/error by stubbing a PnpmError-like object. |
| scopes/dependencies/pnpm/lynx.ts | Migrates pnpm install/resolver/store controller APIs and script/build policy wiring to v11. |
| scopes/dependencies/pnpm/lockfile-deps-graph-converter.ts | Adds async init + ESM loading shim for ESM-only pnpm modules; adjusts types. |
| scopes/dependencies/pnpm/lockfile-deps-graph-converter.spec.ts | Ensures converter init runs before tests; minor structure tweaks. |
| scopes/dependencies/pnpm/load-pnpm-esm.cjs | New CJS shim to import() ESM-only @pnpm/deps.path and @pnpm/lockfile.fs. |
| scopes/dependencies/pnpm/get-registries.ts | Uses config.authConfig in place of rawConfig for credential extraction. |
| scopes/dependencies/pnpm/get-proxy-config.ts | Updates config type import and no-proxy config source. |
| scopes/dependencies/dependency-resolver/package-manager.ts | Updates pnpm peer issues type import to v11 package. |
| scopes/dependencies/dependency-resolver/dependency-resolver.main.runtime.ts | Updates npm resolver import to v11 package. |
| scopes/component/modules/merge-helper/merge-files.ts | Removes unreachable throw err after throwing a new error. |
| e2e/harmony/pnpm-default-hoisting.e2e.ts | Uses ESM-safe dynamic import for modules manifest reading. |
| e2e/harmony/pkg-manager-config.e2e.ts | Removes pnpm-specific capsule rc test section; keeps Yarn case. |
| e2e/harmony/dependency-resolver.e2e.ts | Uses ESM-safe dynamic import for modules manifest reading. |
| components/legacy/e2e-helper/npm-ci-registry.ts | Switches pnpm fetch import to dynamic ESM import (@pnpm/network.fetch). |
| babel.config.js | Ensures Babel doesn’t transform dynamic import() (needed for ESM-only pnpm deps). |
Comment on lines
+33
to
+41
| export async function init(): Promise<void> { | ||
| if (loaded) return; | ||
| const { loadEsm } = require('./load-pnpm-esm.cjs') as { | ||
| loadEsm: () => Promise<{ dp: typeof Dp; getLockfileImporterId: typeof GetLockfileImporterId }>; | ||
| }; | ||
| const m = await loadEsm(); | ||
| dp = m.dp; | ||
| getLockfileImporterId = m.getLockfileImporterId; | ||
| loaded = true; |
Comment on lines
68
to
70
| after(() => { | ||
| npmCiRegistry.destroy(); | ||
| }); |
Comment on lines
1
to
4
| import semver from 'semver'; | ||
| import type { LockfilePackageInfo } from '@pnpm/lockfile.types'; | ||
| import * as dp from '@pnpm/dependency-path'; | ||
| import * as dp from '@pnpm/deps.path'; | ||
|
|
Comment on lines
10
to
13
| (supportNpmCiRegistryTesting ? describe : describe.skip)( | ||
| 'package manager rc file is read from the workspace directory when installation is in a capsule', | ||
| 'workspace .yarnrc.yml is read by Yarn when installation is in a capsule', | ||
| function () { | ||
| this.timeout(0); |
| import { buildDependentsTree } from '@pnpm/reviewing.dependencies-hierarchy'; | ||
| import { renderDependentsTree } from '@pnpm/list'; | ||
| import type { Modules } from '@pnpm/installing.modules-yaml'; | ||
| import { readModulesManifest } from '@pnpm/installing.modules-yaml'; |
Comment on lines
1
to
4
| import semver from 'semver'; | ||
| import type { LockfilePackageInfo } from '@pnpm/lockfile.types'; | ||
| import * as dp from '@pnpm/dependency-path'; | ||
| import * as dp from '@pnpm/deps.path'; | ||
|
|
Comment on lines
10
to
12
| (supportNpmCiRegistryTesting ? describe : describe.skip)( | ||
| 'package manager rc file is read from the workspace directory when installation is in a capsule', | ||
| 'workspace .yarnrc.yml is read by Yarn when installation is in a capsule', | ||
| function () { |
Comment on lines
1
to
4
| import semver from 'semver'; | ||
| import type { LockfilePackageInfo } from '@pnpm/lockfile.types'; | ||
| import * as dp from '@pnpm/dependency-path'; | ||
| import * as dp from '@pnpm/deps.path'; | ||
|
|
Comment on lines
10
to
12
| (supportNpmCiRegistryTesting ? describe : describe.skip)( | ||
| 'package manager rc file is read from the workspace directory when installation is in a capsule', | ||
| 'workspace .yarnrc.yml is read by Yarn when installation is in a capsule', | ||
| function () { |
Comment on lines
+25
to
+31
| // @pnpm/deps.path and @pnpm/lockfile.fs are ESM-only; load them through a .cjs | ||
| // shim so the require() chain in the build capsule's mocha runner doesn't trip | ||
| // on the transitive ESM import. Call `init()` once before invoking the public | ||
| // converters; helpers reach for these module-level slots synchronously. | ||
| let dp!: typeof Dp; | ||
| let getLockfileImporterId!: typeof GetLockfileImporterId; | ||
| let loading: Promise<void> | undefined; |
Comment on lines
10
to
12
| (supportNpmCiRegistryTesting ? describe : describe.skip)( | ||
| 'package manager rc file is read from the workspace directory when installation is in a capsule', | ||
| 'workspace .yarnrc.yml is read by Yarn when installation is in a capsule', | ||
| function () { |
Code Review by Qodo
1. Empty resolver dir
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
428
to
434
| if (dangerouslyAllowAllScripts) { | ||
| if (resolvedNeverBuilt == null) { | ||
| // If neverBuiltDependencies is not explicitly set, use a default list | ||
| // we tell pnpm to allow all scripts to be executed, except the packages listed below. | ||
| resolvedNeverBuilt = ['core-js']; | ||
| // pnpm v11's createAllowBuildFunction ignores allowBuilds when dangerouslyAllowAllBuilds is | ||
| // set, so emit allowBuilds with just the deny entries whenever a deny list exists. | ||
| if (!neverBuiltDependencies?.length) { | ||
| allowBuilds['core-js'] = false; | ||
| return { dangerouslyAllowAllBuilds: true, allowBuilds }; | ||
| } |
There was a problem hiding this comment.
1. Core-js deny ignored 🐞 Bug ⛨ Security
In resolveScriptPolicies(), the branch that sets a default deny for "core-js" also returns dangerouslyAllowAllBuilds: true even though the code comment states pnpm ignores allowBuilds when dangerouslyAllowAllBuilds is set. This makes the default deny ineffective and can unintentionally allow core-js build scripts to run when dangerouslyAllowAllScripts is enabled without an explicit neverBuiltDependencies list.
Agent Prompt
### Issue description
`resolveScriptPolicies()` returns `{ dangerouslyAllowAllBuilds: true, allowBuilds }` in the same branch where it sets `allowBuilds['core-js'] = false`, but the adjacent comment states that pnpm ignores `allowBuilds` when `dangerouslyAllowAllBuilds` is set. This makes the default deny entry ineffective.
### Issue Context
The intent (based on existing policy and comments) is to keep a safe default deny list (at least `core-js`) even when `dangerouslyAllowAllScripts` is enabled.
### Fix Focus Areas
- scopes/dependencies/pnpm/lynx.ts[422-439]
### Suggested fix
Adjust the returned options so that the deny entry is actually honored:
- Do **not** set `dangerouslyAllowAllBuilds` in the code path where you need `allowBuilds` to enforce denies (e.g. for `core-js`).
- Alternatively, if pnpm v11 provides a supported way to express “allow all except these denies”, use that API and add/adjust a regression test for the default `core-js` behavior.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit 03d4bc6 |
| import semver from 'semver'; | ||
| import type { LockfilePackageInfo } from '@pnpm/lockfile.types'; | ||
| import * as dp from '@pnpm/dependency-path'; | ||
| import * as dp from '@pnpm/deps.path'; |
There was a problem hiding this comment.
1. Esm import in cjs 🐞 Bug ☼ Reliability
DependenciesGraph now statically imports @pnpm/deps.path, which this PR itself documents as ESM-only; when Babel transpiles this file to CommonJS, it will become a require() and can crash at runtime with ERR_REQUIRE_ESM. This can break dependency graph operations anywhere @teambit/objects is loaded in the CJS-transpiled execution path (e.g. mocha via babel-register).
Agent Prompt
### Issue description
`scopes/scope/objects/models/dependencies-graph.ts` now statically imports `@pnpm/deps.path`. The PR also states `@pnpm/deps.path` is ESM-only, while the repo’s Babel setup transpiles TS modules to CommonJS (turning `import` into `require()`), which can throw `ERR_REQUIRE_ESM` at runtime.
### Issue Context
- This repo uses `@babel/plugin-transform-modules-commonjs` (via `babel-register`) in at least some execution paths.
- The PR already introduced a CJS shim + dynamic `import()` loader for ESM-only pnpm modules elsewhere, which suggests the same problem applies here.
### Fix Focus Areas
- scopes/scope/objects/models/dependencies-graph.ts[1-120]
- scopes/dependencies/pnpm/load-pnpm-esm.cjs[1-12]
- babel.config.js[1-31]
- babel-register.js[1-1]
### Suggested fix directions
Pick one:
1) **Remove the dependency on `@pnpm/deps.path` here** by implementing a small local parser for the *specific* need (extracting `version` from a depPath string) so this code stays synchronous and CJS-safe.
2) **Mirror the shim approach** used in `lockfile-deps-graph-converter.ts`: add a local loader wrapper for `@pnpm/deps.path` and ensure it is initialized before any `DependenciesGraph` code path that calls `dp.parse()`. If you choose this route, add a clear runtime guard/error message when not initialized (instead of an opaque TypeError).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit 93dcf91 |
Code Review by QodoSorry, something went wrongWe weren't able to complete the code review on our side. Please try again |
Migrate Bit's pnpm integration to the latest v11 (11xx) line of @pnpm/* packages. Adapts the dependency-resolver and pnpm aspects to upstream API changes, loads ESM-only @pnpm modules via .cjs shims, and refreshes the lockfile and workspace.jsonc pins accordingly. All @pnpm/* deps are bumped to their newest 11xx release, including the 1102.x store/resolver/installer line and worker 1100.2.2 (the new content-addressable store generation). lynx.ts therefore keeps the frozenStore flag and the scope-keyed auth translation that the new generation's store.controller / network.auth-header expect. Exception: @pnpm/config.reader is held at 1101.3.1, the last release that depends on @pnpm/config.env-replace 3.x. Newer config.reader needs env-replace ^4.1.0, which bit.cloud's registry mirror does not carry.
|
Code review by qodo was updated up to the latest commit 637e002 |
|
Code review by qodo was updated up to the latest commit 767e5ae |
Route Bit's programmatic pnpm usage through `@pnpm/napi` (NAPI bindings over pnpm v12's Rust engine, pacquet) instead of the pnpm v11 TypeScript `@pnpm/*` engine packages. - `lynx.ts`: `install` now calls `nodeApi.install` (in-memory importers; importer manifests pre-transformed by Bit's readPackage hooks with their workspaceDir, dependency manifests transformed via the synchronous readPackageHook); `resolveRemoteVersion` uses `nodeApi.resolveDependency`; the rebuild closure uses `nodeApi.rebuild`. Auth is resolved from the npmrc-style authConfig into Authorization headers (via the kept `@pnpm/config.reader`) and passed as `authHeaderByUri`. Engine log events bridge to `@pnpm/cli.default-reporter` through the kept `streamParser`. - `packer.ts`: `bit pack` uses `nodeApi.pack`; `load-pnpm-pack.cjs` removed. - `parseBareSpecifier` and the `PeerDependencyIssuesByProjects` type now come from `@pnpm/napi`; the pnpm-error converter duck-types on `.code`/`.hint`. - `workspace.jsonc`: drop the ten engine packages now provided by the Rust binding and add `@pnpm/napi`; keep the data/format packages (lockfile, deps.path, config.reader, logger, cli.default-reporter, types, ...). `getPeerDependencyIssues` degrades to no issues while that engine op is unimplemented in the binding.
feat(dependency-resolver): use the Rust pnpm engine via @pnpm/napi
zkochan
changed the title
Comment on lines
401
to
418
| await installsRunning[rootDir]; | ||
| await restartWorkerPool(); | ||
| installsRunning[rootDir] = mutateModules(packagesToBuild, opts); | ||
| const installResult = await installsRunning[rootDir]; | ||
| depsRequiringBuild = installResult.depsRequiringBuild?.sort(); | ||
| if (depsRequiringBuild != null) { | ||
| await addDepsRequiringBuildToLockfile(rootDir, depsRequiringBuild); | ||
| installsRunning[rootDir] = nodeApi.install(installOptions, onLog, readPackageHookForDeps); | ||
| const installResult: nodeApi.InstallResult = await installsRunning[rootDir]; | ||
| resolvedStoreDir = installResult.storeDir; | ||
| const sortedDepsRequiringBuild = installResult.depsRequiringBuild?.sort(); | ||
| if (sortedDepsRequiringBuild != null) { | ||
| await addDepsRequiringBuildToLockfile(rootDir, sortedDepsRequiringBuild); | ||
| depsRequiringBuild = sortedDepsRequiringBuild as unknown as DepPath[]; | ||
| } | ||
| dependenciesChanged = | ||
| installResult.stats.added + installResult.stats.removed + installResult.stats.linkedToRoot > 0; | ||
| delete installsRunning[rootDir]; | ||
| } catch (err: any) { | ||
| if (logger) { | ||
| logger.warn('got an error from pnpm mutateModules function', err); | ||
| logger.warn('got an error from the pnpm install function', err); | ||
| } | ||
| throw pnpmErrorToBitError(err); | ||
| } finally { |
There was a problem hiding this comment.
1. Install lock not cleared 🐞 Bug ☼ Reliability
In lynx.install(), installsRunning[rootDir] is only deleted on the success path; if nodeApi.install() rejects, the rejected promise remains cached and subsequent installs for the same rootDir will immediately fail at await installsRunning[rootDir]. This prevents retries in the same process after a single install failure.
Agent Prompt
### Issue description
`installsRunning[rootDir]` is used to serialize installs, but it is only deleted on the success path. If `nodeApi.install(...)` throws/rejects, the rejected promise remains stored, so the next call will `await installsRunning[rootDir]` and fail immediately without attempting a new install.
### Issue Context
This is a per-process cache/lock. A single transient error (network, registry, etc.) can permanently block subsequent installs for that workspace root until the process restarts.
### Fix Focus Areas
- scopes/dependencies/pnpm/lynx.ts[389-420]
### Implementation notes
- Capture the current promise in a local variable (`const p = nodeApi.install(...)`), assign it to the map, and in `finally` do `if (installsRunning[rootDir] === p) delete installsRunning[rootDir]`.
- Ensure the entry is cleared on both success and failure, while still preserving serialization semantics.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+403
to
+440
| const installResult: nodeApi.InstallResult = await installsRunning[rootDir]; | ||
| resolvedStoreDir = installResult.storeDir; | ||
| const sortedDepsRequiringBuild = installResult.depsRequiringBuild?.sort(); | ||
| if (sortedDepsRequiringBuild != null) { | ||
| await addDepsRequiringBuildToLockfile(rootDir, sortedDepsRequiringBuild); | ||
| depsRequiringBuild = sortedDepsRequiringBuild as unknown as DepPath[]; | ||
| } | ||
| dependenciesChanged = | ||
| installResult.stats.added + installResult.stats.removed + installResult.stats.linkedToRoot > 0; | ||
| delete installsRunning[rootDir]; | ||
| } catch (err: any) { | ||
| if (logger) { | ||
| logger.warn('got an error from pnpm mutateModules function', err); | ||
| logger.warn('got an error from the pnpm install function', err); | ||
| } | ||
| throw pnpmErrorToBitError(err); | ||
| } finally { | ||
| stopReporting?.(); | ||
| await finishWorkers(); | ||
| } | ||
| } | ||
| return { | ||
| dependenciesChanged, | ||
| rebuild: async (rebuildOpts) => { | ||
| // The Rust engine's rebuild rebuilds every build-needing package and does not | ||
| // support the pending / skipIfHasSideEffectsCache selectors of the old engine. | ||
| rebuild: async () => { | ||
| let stopReporting: Function | undefined; | ||
| const _opts = { | ||
| ...opts, | ||
| ...rebuildOpts, | ||
| cacheDir, | ||
| } as any; // eslint-disable-line @typescript-eslint/no-explicit-any | ||
| if (!_opts.hidePackageManagerOutput) { | ||
| if (!options.hidePackageManagerOutput) { | ||
| stopReporting = initReporter({ | ||
| appendOnly: true, | ||
| hideLifecycleOutput: true, | ||
| }); | ||
| } | ||
| try { | ||
| await rebuild.handler(_opts, []); | ||
| await nodeApi.rebuild(installOptions, onLog, undefined); | ||
| } finally { | ||
| stopReporting?.(); | ||
| } | ||
| }, | ||
| storeDir: storeController.dir, | ||
| storeDir: resolvedStoreDir ?? '', |
There was a problem hiding this comment.
2. Rebuild uses stale storedir 🐞 Bug ≡ Correctness
lynx.install() updates resolvedStoreDir from installResult.storeDir, but rebuild() calls nodeApi.rebuild() with the original installOptions object whose storeDir field is never updated. When storeDir input is undefined or the engine resolves/normalizes it, rebuild can run against the wrong store location.
Agent Prompt
### Issue description
`installResult.storeDir` is captured into `resolvedStoreDir` and returned, but `rebuild()` reuses the precomputed `installOptions` object, whose `storeDir` property remains the original input `storeDir` (which may be `undefined`). This can make rebuild use a different store directory than install used.
### Issue Context
The code already anticipates storeDir potentially being missing/changed (`resolvedStoreDir ?? ''`). Any mismatch can lead to rebuild operating on the wrong store content or failing to locate it.
### Fix Focus Areas
- scopes/dependencies/pnpm/lynx.ts[334-387]
- scopes/dependencies/pnpm/lynx.ts[401-442]
### Implementation notes
- After a successful install, either:
- mutate `installOptions.storeDir = resolvedStoreDir` (or `installResult.storeDir`) before returning, or
- create a separate options object for rebuild: `{ ...installOptions, storeDir: resolvedStoreDir }`.
- Keep behavior consistent for dryRun (no installResult).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+136
to
140
| const packResult = await nodeApi.pack({ dir: cwd }); | ||
| const tgzName = path.basename(packResult.tarballPath); | ||
| this.logger.debug(`successfully packed tarball at ${cwd}`); | ||
| const tgzOriginPath = path.join(cwd, tgzName); | ||
| const tgzOriginPath = path.isAbsolute(packResult.tarballPath) ? packResult.tarballPath : path.join(cwd, tgzName); | ||
| let tarPath = path.join(outputPath, tgzName); |
There was a problem hiding this comment.
3. Pack tarballpath mishandled 🐞 Bug ≡ Correctness
pnpmPack() reconstructs the origin tarball path as cwd + basename(tarballPath) when tarballPath is relative, dropping any relative directory segments. If nodeApi.pack() returns a relative tarball path that includes subdirectories, fs.move() will look for the wrong source path and fail.
Agent Prompt
### Issue description
When `packResult.tarballPath` is relative, the code computes:
- `tgzName = basename(tarballPath)`
- `tgzOriginPath = join(cwd, tgzName)`
This discards any relative folder components included in `tarballPath`.
### Issue Context
The subsequent `fs.move(tgzOriginPath, tarPath)` relies on `tgzOriginPath` accurately pointing to the tarball created by pnpm.
### Fix Focus Areas
- scopes/pkg/pkg/packer.ts[136-160]
### Implementation notes
- Compute origin path as:
- `const origin = path.isAbsolute(tarballPath) ? tarballPath : path.join(cwd, tarballPath)`
- Keep `tgzName` for naming, but don’t rebuild the origin path from basename.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit cd274da |
…deep' - @pnpm/napi 12.0.0-alpha.3 -> 12.0.0-alpha.4, which adds the linkWorkspacePackages install option (pnpm/pnpm#12844). - lynx.ts sets linkWorkspacePackages: 'deep' so bare-semver deps on workspace components — including auto-installed peers that name a sibling component — resolve from the workspace instead of 404ing against the registry. Fixes the peer-dependency-component e2e failures.
Comment on lines
+18
to
+22
| let credentials = getCredentialsByURI(config.authConfig, uri); | ||
| let originalAuthConfig = getOriginalAuthConfigByUri(config.authConfig, uri); | ||
| if (isEmpty(credentials)) { | ||
| credentials = getCredentialsByURI(config.rawConfig, switchTrailingSlash(uri)); | ||
| originalAuthConfig = getOriginalAuthConfigByUri(config.rawConfig, switchTrailingSlash(uri)); | ||
| credentials = getCredentialsByURI(config.authConfig, switchTrailingSlash(uri)); | ||
| originalAuthConfig = getOriginalAuthConfigByUri(config.authConfig, switchTrailingSlash(uri)); |
There was a problem hiding this comment.
1. Auth registry key missing 🐞 Bug ≡ Correctness
getRegistries() now passes config.authConfig into getOriginalAuthConfigByUri(), but that helper reads config.registry to compute the default registry nerf-dart; if authConfig doesn’t carry a registry string, the default-auth fallback logic can crash or behave incorrectly.
Agent Prompt
### Issue description
`getRegistries()` calls `getOriginalAuthConfigByUri(config.authConfig, uri)`, but `getOriginalAuthConfigByUri()` expects the passed object to contain `registry` (used as the default registry URI). With `@pnpm/config.reader`, `Config.registries` is already the authoritative source of registry URLs, so relying on `authConfig.registry` is brittle and may be `undefined`.
### Issue Context
This impacts building `Registry` objects (default and scoped) used across installs/resolution.
### Fix Focus Areas
- scopes/dependencies/pnpm/get-registries.ts[13-46]
### Suggested fix
Option A (preferred, explicit):
1) Change `getOriginalAuthConfigByUri` signature to accept `defaultRegistryUri: string`.
2) Call it as `getOriginalAuthConfigByUri(config.authConfig, uri, config.registries.default)` (or equivalent default-registry entry).
3) Replace `toNerfDart(config.registry)` with `toNerfDart(defaultRegistryUri)`.
Option B (minimal change):
- Call `getOriginalAuthConfigByUri({ ...config.authConfig, registry: config.registries.default }, uri)` to ensure the expected `registry` key exists.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+206
to
+212
| } catch (err: any) { | ||
| if (err?.code === 'ERR_PNPM_NAPI_UNIMPLEMENTED') { | ||
| // TODO: getPeerDependencyIssues not yet implemented in the Rust engine. | ||
| // Returning no issues keeps installs working until the binding lands it. | ||
| return {}; | ||
| } | ||
| throw pnpmErrorToBitError(err); |
There was a problem hiding this comment.
2. Peer issues suppressed silently 🐞 Bug ◔ Observability
lynx.getPeerDependencyIssues() returns {} for ERR_PNPM_NAPI_UNIMPLEMENTED, which makes
dependency-resolver.getMissingPeerDependencies() return undefined and causes the install flow to
report “No missing peer dependencies found” even when peer detection is actually unavailable.
Agent Prompt
### Issue description
When `@pnpm/napi` throws `ERR_PNPM_NAPI_UNIMPLEMENTED`, `getPeerDependencyIssues()` currently returns `{}`. Downstream, Bit interprets this as “no peer issues,” so features like `--add-missing-peers` can silently become a no-op without any indication that peer-issue calculation is unsupported.
### Issue Context
- `dependency-resolver.getMissingPeerDependencies()` reads `peerDependencyIssues['.']?.intersections`.
- The install command uses falsy result to print “No missing peer dependencies found.”
### Fix Focus Areas
- scopes/dependencies/pnpm/lynx.ts[177-213]
- scopes/dependencies/dependency-resolver/dependency-resolver.main.runtime.ts[896-922]
### Suggested fix
- In the `ERR_PNPM_NAPI_UNIMPLEMENTED` branch, emit a warning (once per process) indicating peer-dependency detection is currently unavailable with the pnpm napi engine.
- Return a safe, expected shape for the caller to avoid `undefined`, e.g. `{ '.': { intersections: {} } }` (or whatever minimal `PeerDependencyIssuesByProjects` shape the rest of the code expects).
- Alternatively, plumb a flag to allow throwing a clear BitError when `addMissingPeers` is requested, instead of silently returning empty results.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit a76f6b7 |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment on lines
25
to
28
| import { BitError } from '@teambit/bit-error'; | ||
| import { BIT_ROOTS_DIR } from '@teambit/legacy.constants'; | ||
| import type { MutatedProject, InstallOptions, PeerDependencyIssuesByProjects, ProjectOptions } from '@pnpm/core'; | ||
| import { mutateModules } from '@pnpm/core'; | ||
| import * as pnpm from '@pnpm/core'; | ||
| import type { ClientOptions } from '@pnpm/client'; | ||
| import { createClient } from '@pnpm/client'; | ||
| import { restartWorkerPool, finishWorkers } from '@pnpm/worker'; | ||
| import { createPkgGraph } from '@pnpm/workspace.pkgs-graph'; | ||
| import { readWantedLockfile, writeWantedLockfile } from '@pnpm/lockfile.fs'; | ||
| import { type LockfileFile, type LockfileObject } from '@pnpm/lockfile.types'; |
There was a problem hiding this comment.
1. Cjs requires esm module 🐞 Bug ☼ Reliability
TS files are transpiled to CommonJS (require()), but pnpm integration code still statically imports @pnpm/lockfile.fs even though this PR documents it as ESM-only. This can crash at module-load time with ERR_REQUIRE_ESM during installs/prune/lockfile operations in CJS contexts (e.g. mocha/capsules).
Agent Prompt
## Issue description
`@pnpm/lockfile.fs` is treated as ESM-only in this PR, but several TypeScript modules still import it statically. Since the repo transpiles TS to CommonJS (`@babel/plugin-transform-modules-commonjs`), these imports become `require('@pnpm/lockfile.fs')` and can throw `ERR_REQUIRE_ESM` at runtime.
## Issue Context
The PR already introduces a `.cjs` shim (`load-pnpm-esm.cjs`) specifically because `@pnpm/lockfile.fs` is ESM-only. That mitigation is currently incomplete because other runtime code paths still use static imports.
## Fix Focus Areas
- scopes/dependencies/pnpm/lynx.ts[24-33]
- scopes/dependencies/pnpm/pnpm.package-manager.ts[24-35]
- scopes/dependencies/pnpm/pnpm-prune-modules.ts[1-17]
- scopes/dependencies/pnpm/load-pnpm-esm.cjs[3-12]
- babel.config.js[19-26]
### Suggested implementation direction
- Remove top-level runtime imports from `@pnpm/lockfile.fs`.
- Replace them with a memoized dynamic loader (e.g. `let lockfileFsPromise; async function loadLockfileFs(){...}`) that uses `import('@pnpm/lockfile.fs')`, or extend the existing `load-pnpm-esm.cjs` shim to export the additional functions you need (`readWantedLockfile`, `writeWantedLockfile`, `readCurrentLockfile`, etc.).
- Update call sites to `const { readWantedLockfile } = await loadLockfileFs()` (or equivalent) before use.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit 35185fd |
parse-package-name returns an empty string for a spec with no version (e.g. 'is-positive', '@teambit/react.react-env'). The napi resolver rejects an empty bareSpecifier with "isn't supported by any available resolver" — only undefined (or an explicit range) resolves as latest. This broke every 'bit install <pkg>' / 'bit create' path that resolves a package without a version (~60 e2e failures on CI). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Code review by qodo was updated up to the latest commit ee19e33 |
Carries the engine fixes for Bit's e2e failures (pnpm/pnpm#12899): root-component member linking without on-disk manifests, injectedDeps in .modules.yaml, hoisted-linker peered-snapshot and importer-subtree materialization, out-of-root declared importers, lockfile-excluded link: deps, empty-specifier resolution, hoistWorkspacePackages name-links, and an implemented getPeerDependencyIssues. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment on lines
1476
to
1482
| isValidVersionSpecifier(spec: string): boolean { | ||
| // parseBareSpecifier returns null for empty/unparsable input. | ||
| return ( | ||
| parseBareSpecifier( | ||
| spec, | ||
| 'pkgname', // This argument is the package but we don't need it | ||
| 'latest', | ||
| 'https://registry.npmjs.org/' | ||
| 'pkgname' // This argument is the package name but we don't need it | ||
| ) != null |
There was a problem hiding this comment.
1. Eager napi import 🐞 Bug ☼ Reliability
dependency-resolver.main.runtime.ts now imports parseBareSpecifier from @pnpm/napi at module scope, forcing the native binding to load whenever the dependency-resolver aspect is loaded. This can increase startup cost and can hard-fail process initialization in environments where the native binding cannot be loaded, even if pnpm isn’t the active package manager.
Agent Prompt
### Issue description
`dependency-resolver.main.runtime.ts` performs a module-scope import from `@pnpm/napi` and uses it in `isValidVersionSpecifier()`. This eagerly loads the native binding whenever the dependency-resolver aspect is loaded, which can slow startup and can crash early if the native module can’t be loaded.
### Issue Context
Other pnpm integration points in this repo explicitly lazy-load pnpm internals for performance, so this eager import is inconsistent and increases blast radius.
### Fix Focus Areas
- Replace the module-scope `@pnpm/napi` import with a non-native, lightweight validator (e.g. `semver.valid/validRange` + allow-list for tags/aliases/URLs), OR
- If `parseBareSpecifier` is required for parity, load it lazily inside `isValidVersionSpecifier()` with a guarded `require()`/cached getter and provide a safe fallback when the binding fails to load.
- scopes/dependencies/dependency-resolver/dependency-resolver.main.runtime.ts[36-46]
- scopes/dependencies/dependency-resolver/dependency-resolver.main.runtime.ts[1469-1484]
- scopes/dependencies/pnpm/pnpm.package-manager.ts[127-134]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
4
to
11
| export function getProxyConfig(config: Config): PackageManagerProxyConfig { | ||
| const httpProxy = config.httpProxy; | ||
| const httpsProxy = config.httpsProxy || httpProxy; | ||
| const proxyConfig: PackageManagerProxyConfig = { | ||
| httpProxy, | ||
| httpsProxy, | ||
| noProxy: config.rawConfig.noproxy, | ||
| noProxy: config.authConfig.noproxy, | ||
| }; |
There was a problem hiding this comment.
2. Noproxy from authconfig 🐞 Bug ≡ Correctness
getProxyConfig() sets noProxy from config.authConfig.noproxy, which is not the typed proxy-bypass setting and may not contain the user’s .npmrc no-proxy value. As a result, dependency downloads can incorrectly go through the proxy even when the user configured domains to bypass it.
Agent Prompt
### Issue description
`getProxyConfig()` sources `noProxy` from `config.authConfig.noproxy`. `authConfig` is used primarily for auth/credential configuration, and using it for proxy-bypass settings risks ignoring `.npmrc` `no-proxy`/`noproxy` configuration.
### Issue Context
Bit’s dependency-resolver supports `noProxy` explicitly and threads it through proxy configuration; if `getProxyConfig()` fails to populate it, proxy bypass won’t work.
### Fix Focus Areas
- Prefer the typed config field for proxy bypass if available (e.g. `config.noProxy`).
- If pnpm’s config reader still exposes raw keys, add a safe fallback chain (e.g. `config.noProxy ?? config.authConfig['no-proxy'] ?? config.authConfig.noproxy`).
- scopes/dependencies/pnpm/get-proxy-config.ts[1-13]
- scopes/dependencies/dependency-resolver/dependency-resolver-workspace-config.ts[55-59]
- scopes/dependencies/dependency-resolver/dependency-resolver.main.runtime.ts[805-809]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit f5c3302 |
…installs The Rust engine rewrites pnpm-lock.yaml from its typed model, which does not round-trip Bit's bit: block — the v11 TS engine mutated the loaded object in place, so attributes like restoredFromModel survived installs. Capture the block before the engine runs and re-assert it (merged with the fresh depsRequiringBuild) afterwards. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment on lines
+55
to
+75
| "@pnpm/cli.default-reporter": "1100.3.3", | ||
| "@pnpm/colorize-semver-diff": "1.0.1", | ||
| "@pnpm/config": "1004.11.0", | ||
| "@pnpm/core": "1016.2.0", | ||
| "@pnpm/default-reporter": "1002.1.14", | ||
| "@pnpm/dependency-path": "1001.1.10", | ||
| "@pnpm/error": "1000.1.0", | ||
| "@pnpm/fetch": "1001.0.0", | ||
| "@pnpm/list": "^1000.3.4", | ||
| "@pnpm/lockfile.filtering": "^1001.0.31", | ||
| "@pnpm/lockfile.fs": "^1001.1.32", | ||
| "@pnpm/lockfile.types": "^1002.1.0", | ||
| "@pnpm/logger": "1001.0.1", | ||
| "@pnpm/modules-yaml": "1002.0.1", | ||
| "@pnpm/config.parse-overrides": "1100.0.2", | ||
| "@pnpm/config.reader": "1101.3.1", | ||
| "@pnpm/deps.inspection.list": "1100.0.20", | ||
| "@pnpm/deps.inspection.tree-builder": "1100.0.17", | ||
| "@pnpm/deps.path": "1100.0.8", | ||
| "@pnpm/error": "1100.0.1", | ||
| "@pnpm/installing.modules-yaml": "1100.0.9", | ||
| "@pnpm/lockfile.filtering": "1100.1.8", | ||
| "@pnpm/lockfile.fs": "1100.1.7", | ||
| "@pnpm/lockfile.types": "1100.0.12", | ||
| "@pnpm/logger": "1100.0.0", | ||
| "@pnpm/network.ca-file": "3.0.3", | ||
| "@pnpm/network.fetch": "1100.1.4", | ||
| "@pnpm/napi": "12.0.0-alpha.6", | ||
| "@pnpm/node-fetch": "^1.0.0", | ||
| "@pnpm/npm-resolver": "1005.2.3", | ||
| "@pnpm/package-store": "1007.1.6", | ||
| "@pnpm/parse-overrides": "1001.0.4", | ||
| "@pnpm/plugin-commands-publishing": "1000.3.16", | ||
| "@pnpm/plugin-commands-rebuild": "1008.0.13", | ||
| "@pnpm/plugin-trusted-deps": "0.2.2", | ||
| "@pnpm/registry-mock": "3.48.0", | ||
| "@pnpm/reviewing.dependencies-hierarchy": "^1001.3.4", | ||
| "@pnpm/semver-diff": "1.1.0", | ||
| "@pnpm/sort-packages": "1000.0.16", | ||
| "@pnpm/store-connection-manager": "1002.3.19", | ||
| "@pnpm/types": "1001.3.0", | ||
| "@pnpm/worker": "1000.6.7", | ||
| "@pnpm/workspace.pkgs-graph": "1000.0.39", | ||
| "@pnpm/types": "1101.3.2", |
There was a problem hiding this comment.
1. Node engine mismatch 🐞 Bug ☼ Reliability
The PR introduces new @pnpm/* 11xx/12.x deps that declare Node >=22.13 in pnpm-lock.yaml, while the repo still advertises "engines.node": ">=12.22.0" in package.json. This creates misleading compatibility metadata and can cause installs/tooling to fail unexpectedly on Node versions that appear supported by the repo.
Agent Prompt
### Issue description
The repository's declared Node engine requirement is out of sync with the actual Node version required by the upgraded pnpm internal dependencies.
- `package.json` still declares `engines.node: >=12.22.0`.
- The PR adds pnpm v11/v12-era packages (11xx/12.x) which, per `pnpm-lock.yaml`, require Node `>=22.13`.
- `workspace.jsonc` already pins `nodeVersion: 22.22.0` with `engineStrict: true`, reinforcing that the real minimum Node version is much higher than `package.json` claims.
This mismatch misleads consumers and CI/tooling that relies on `package.json#engines`.
### Issue Context
The pnpm internal dependency set introduced by this PR appears to be Node-22+ only (lockfile `engines` fields), but the repo-level engine metadata wasn’t updated accordingly.
### Fix Focus Areas
- package.json[5-7]
- workspace.jsonc[720-722]
- pnpm-lock.yaml[32843-32858]
### Suggested fix
Update `package.json#engines.node` to reflect the real minimum Node version required by the new pnpm internals (at least `>=22.13`, or align exactly with the workspace’s pinned `22.22.0` policy), and ensure any documentation/CI configuration that depends on `engines` remains consistent.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit 2348960 |
Carries the plain file: importer-ref serialization fix (pnpm/pnpm#12924), which resolves the bit tag crash in the lockfile-to-graph converter (Cannot read properties of undefined) under BIT_FEATURES=deps-graph. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… graph calc
calcDependenciesGraph scrubs other workspace components from the cloned
importers before filtering the lockfile, so a sibling entry never leaks
into the component's graph. In the Rust engine's per-importer capsule
layout, though, an injected sibling ("file:" ref) in the component's own
importer is a real direct dependency — the converter rewrites it to the
component's semver id — so only non-file entries (and entries in other
importers, e.g. the capsule/workspace root) are scrubbed now. The v11
layouts never record file: siblings in a component's own importer, so
this is a no-op for lockfiles written by the TS engine.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment on lines
+412
to
+421
| const preInstallBitAttrs = await readBitLockfileAttrs(rootDir); | ||
| installsRunning[rootDir] = nodeApi.install(installOptions, onLog, readPackageHookForDeps); | ||
| const installResult: nodeApi.InstallResult = await installsRunning[rootDir]; | ||
| resolvedStoreDir = installResult.storeDir; | ||
| const sortedDepsRequiringBuild = installResult.depsRequiringBuild?.sort(); | ||
| if (sortedDepsRequiringBuild != null || preInstallBitAttrs != null) { | ||
| await addBitAttributesToLockfile(rootDir, { | ||
| ...preInstallBitAttrs, | ||
| ...(sortedDepsRequiringBuild != null ? { depsRequiringBuild: sortedDepsRequiringBuild } : {}), | ||
| }); |
There was a problem hiding this comment.
1. Stale build-deps lockfile flag 🐞 Bug ≡ Correctness
lynx.install() re-applies the pre-install bit: lockfile block but only overwrites depsRequiringBuild when installResult.depsRequiringBuild is non-null; if the engine omits this field when nothing needs a build, the old list is preserved. This stale list is later consumed by convertLockfileToGraph() to mark packages as requiresBuild, producing incorrect dependency graph metadata.
Agent Prompt
### Issue description
`depsRequiringBuild` can become stale across installs. If the Rust engine returns `depsRequiringBuild: undefined` (as opposed to `[]`) for an install where nothing requires build, the current merge logic writes back the previous lockfile’s `bit.depsRequiringBuild` unchanged.
### Issue Context
- `addBitAttributesToLockfile()` merges `attrs` over the existing `lockfile.bit` object, so missing keys are preserved.
- `lockfile-deps-graph-converter.ts` uses `lockfile.bit?.depsRequiringBuild` to set `graphPkg.requiresBuild = true`.
### Fix Focus Areas
- scopes/dependencies/pnpm/lynx.ts[412-425]
- scopes/dependencies/pnpm/lynx.ts[749-755]
### Suggested fix
When re-asserting the `bit:` block after install, explicitly set `depsRequiringBuild` to an empty array when the engine returns it as nullish.
For example:
- Compute `const deps = (installResult.depsRequiringBuild ?? []).slice().sort();`
- Call `addBitAttributesToLockfile(rootDir, { ...preInstallBitAttrs, depsRequiringBuild: deps })`
- Keep returning `depsRequiringBuild` to callers based on `deps.length` (if you want to preserve the current `undefined` return behavior) but ensure the lockfile always gets an accurate array.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit 0a5e620 |
…nces The engine hands the readPackage hook a workspace project's importer manifest when it resolves that project as an injected "file:" dependency. That manifest was already stripped of workspace-sibling deps by the up-front with-dir transform (Bit wires siblings itself inside importers), so the injected instance lost its component deps — and with them the component graph edges recorded at tag time (the deps-graph e2e "should replace pending version in direct dependency" cluster). The next @pnpm/napi release passes the lockfile-root-relative directory of a directory resolution as the hook's second argument; use it to substitute the project's raw manifest for its dependency instances, mirroring how pnpm's TS engine applies the hook chain contextually to raw project manifests. With the current binding the argument is simply absent and behavior is unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment on lines
+162
to
+174
| const mergedAuthConfig = Object.assign({}, pnpmConfig.config.authConfig, authConfig) as Record<string, unknown>; | ||
| const authHeaderByUri = buildAuthHeaderByUri(mergedAuthConfig); | ||
| const registriesMap = registries.toMap(); | ||
| const resolve: ResolveFunction = (wantedDep, resolveOpts) => | ||
| nodeApi.resolveDependency(wantedDep, { | ||
| dir: resolveOpts.projectDir ?? resolveOpts.lockfileDir ?? '', | ||
| cacheDir, | ||
| registries: registriesMap, | ||
| authHeaderByUri, | ||
| proxyConfig: proxyConfig ? toNodeApiProxyConfig(proxyConfig) : undefined, | ||
| networkConfig: networkConfig ? toNodeApiNetworkConfig(networkConfig) : undefined, | ||
| fullMetadata, | ||
| }); |
There was a problem hiding this comment.
1. Empty resolver dir 🐞 Bug ≡ Correctness
generateResolverAndFetcher() forwards `dir: resolveOpts.projectDir ?? resolveOpts.lockfileDir ?? '' into nodeApi.resolveDependency`, so callers that pass empty strings end up invoking the resolver with an empty directory. convertGraphToLockfile() currently calls resolve() with lockfileDir: '' and projectDir: '', so lockfile generation can fail or resolve using config from an unintended working directory.
Agent Prompt
## Issue description
`nodeApi.resolveDependency` is invoked with `dir` derived via nullish coalescing (`??`). Empty strings are not nullish, so `dir` can become `''`, which is not a meaningful base directory for resolution/config discovery.
## Issue Context
At least one internal caller (`convertGraphToLockfile`) passes empty strings for `projectDir`/`lockfileDir`, which now propagate into the resolver due to the new implementation.
## Fix Focus Areas
- scopes/dependencies/pnpm/lynx.ts[147-175]
- scopes/dependencies/pnpm/lockfile-deps-graph-converter.ts[369-386]
## Concrete fix
1) In `generateResolverAndFetcher`, treat empty string as missing by using `||` (or explicitly checking for a non-empty string), and fall back to a safe default:
```ts
const dir = resolveOpts.projectDir || resolveOpts.lockfileDir || process.cwd();
...
nodeApi.resolveDependency(wantedDep, { dir, ... })
```
2) In `convertGraphToLockfile`, stop passing `''` for directory fields; pass the real `rootDir` (or `projectDir`) so the resolver runs with the correct context.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit 9c45e72 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
@pnpm/*deps inworkspace.jsoncto v11's@pnpm/<domain>.<leaf>naming (e.g.@pnpm/core→@pnpm/installing.deps-installer,@pnpm/config→@pnpm/config.reader), bump in-monorepo packages to1100.0.0.mutateModulesnow usesallowBuilds: Record<string, boolean | string>(plusdangerouslyAllowAllBuilds) in place of the removedneverBuiltDependencies/onlyBuiltDependencies/ignoredBuiltDependenciestrio.Config.rawConfig→Config.authConfigrename; read network settings (maxSockets,fetchRetries, etc.) from typedConfigfields instead of rawConfig ini lookups.configByUri: Record<string, RegistryConfig>shape forCreateStoreControllerOptions; switchgenerateResolverAndFetchertocreateResolversince both callers only use.resolve(avoids the now-requiredstoreIndexoncreateClient).sortPackages→sortProjects,createPkgGraph→createProjectsGraph,createOrConnectStoreController→createStoreController; supply now-requiredglobalVirtualStoreDirtogetPeerDependencyIssues.Test plan
npm run check-typespassesbit compile— 310/310 components compileinstall new dependencies (using pnpm)e2e — 5/5 passinstall missing dependencies (when all packages exist)e2e — 2/2 pass (covers add/tag/export/import/reinstall)skipping compilation on installe2e — 2/2 pass