deps: migrate pnpm internals to v11 (1100.x)#10293
Open
zkochan wants to merge 2 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. ESM import in CJS
|
![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 |
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