fix(config): warn when package.json has a legacy "pnpm" field with migrated settings#11680
Merged
zkochan merged 2 commits intoMay 17, 2026
Merged
Conversation
|
💖 Thanks for opening this pull request! 💖 |
📝 WalkthroughWalkthroughConfig reader now detects legacy top-level ChangesLegacy pnpm field migration warning
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
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 unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
config/reader/test/index.ts (1)
1630-1643: ⚡ Quick winAdd a negative test for non-migrated
pnpmkeys.Please add one fixture/test that includes a non-migrated key under
pnpm(for examplepnpm.foo) and asserts no warning is emitted. This locks in the “warn only for migrated keys” contract from the issue/PR objective.Proposed test shape
+test('do not warn for non-migrated keys inside package.json pnpm field', async () => { + const prefix = f.find('pkg-with-non-migrated-pnpm-field') + const { warnings } = await getConfig({ + cliOptions: { dir: prefix }, + packageManager: { + name: 'pnpm', + version: '1.0.0', + }, + }) + + expect(warnings).toStrictEqual([]) +})🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@config/reader/test/index.ts` around lines 1630 - 1643, Add a negative test alongside the existing 'return a warning if a package.json has a legacy "pnpm" field with migrated settings' case that creates a fixture package.json containing a non-migrated pnpm key (e.g. "pnpm": { "foo": "bar" }) and calls getConfig with the same cliOptions and packageManager stub; assert that the returned warnings array is empty (expect(warnings).toHaveLength(0) or toStrictEqual([])). Use the same test harness/syntax as the existing test, name the test to indicate "no warning for non-migrated pnpm keys", and reference getConfig and the fixture name used for locating the package (e.g. f.find('pkg-with-non-migrated-pnpm-key')) so the test runner can find the package.json.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@config/reader/test/index.ts`:
- Around line 1630-1643: Add a negative test alongside the existing 'return a
warning if a package.json has a legacy "pnpm" field with migrated settings' case
that creates a fixture package.json containing a non-migrated pnpm key (e.g.
"pnpm": { "foo": "bar" }) and calls getConfig with the same cliOptions and
packageManager stub; assert that the returned warnings array is empty
(expect(warnings).toHaveLength(0) or toStrictEqual([])). Use the same test
harness/syntax as the existing test, name the test to indicate "no warning for
non-migrated pnpm keys", and reference getConfig and the fixture name used for
locating the package (e.g. f.find('pkg-with-non-migrated-pnpm-key')) so the test
runner can find the package.json.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 12b8ab12-803c-4be7-aa68-3949b82d3079
📒 Files selected for processing (4)
.changeset/warn-deprecated-pnpm-field-11677.mdconfig/reader/src/index.tsconfig/reader/test/fixtures/pkg-with-legacy-pnpm-field/package.jsonconfig/reader/test/index.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use Standard Style with modifications: trailing commas are used, functions are preferred over classes, functions are declared after they are used (hoisting is relied upon).
Functions should have no more than two or three arguments. If a function needs more parameters, use a single options object instead.
Maintain import order: (1) Standard libraries, (2) External dependencies (sorted alphabetically), (3) Relative imports.
Files:
config/reader/test/index.tsconfig/reader/src/index.ts
🧠 Learnings (2)
📚 Learning: 2026-05-05T23:03:04.286Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11479
File: __utils__/scripts/package.json:6-9
Timestamp: 2026-05-05T23:03:04.286Z
Learning: The pattern cross-env NODE_OPTIONS="$NODE_OPTIONS ..." in package.json scripts is an established convention in the pnpm/pnpm repository and is used across many packages (e.g., fs/hard-link-dir, worker, __utils__/scripts). Do not flag this as a cross-platform issue in individual files; if a change is needed, apply it as a repo-wide change in a separate PR. Scope this guidance to all package.json files in the repo; use the minimatch pattern '**/package.json' to identify relevant files and review changes at the repository level rather than per-file.
Applied to files:
config/reader/test/fixtures/pkg-with-legacy-pnpm-field/package.json
📚 Learning: 2026-05-14T09:04:00.133Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11622
File: resolving/npm-resolver/test/publishedBy.test.ts:350-354
Timestamp: 2026-05-14T09:04:00.133Z
Learning: In the pnpm/pnpm repository, ESLint is the authoritative style linter. Do not raise review findings for missing trailing commas in multiline function calls (e.g., `fs.writeFileSync(...)`) when this repo’s ESLint configuration does not report them and lint passes. Prefer deferring to the ESLint results for this specific trailing-comma rule rather than enforcing it manually in code review.
Applied to files:
config/reader/test/index.tsconfig/reader/src/index.ts
🔇 Additional comments (3)
config/reader/src/index.ts (1)
407-410: LGTM!Also applies to: 755-781
.changeset/warn-deprecated-pnpm-field-11677.md (1)
1-7: LGTM!config/reader/test/fixtures/pkg-with-legacy-pnpm-field/package.json (1)
1-10: LGTM!
baa4ddd to
9a53229
Compare
In v11, pnpm stopped reading settings from the `pnpm` field of package.json (pnpm#10086). Most former pnpm-field settings now live in `pnpm-workspace.yaml`; a few (e.g. `onlyBuiltDependencies`, `executionEnv`) were removed entirely. Until now the old field was silently ignored, so users upgrading from v10 had no signal that their overrides or patched dependencies had stopped taking effect. Emit a warning whenever the `pnpm` field contains any key that pnpm no longer reads from package.json. The check is an allowlist (only `pnpm.app`, consumed by `pnpm pack-app`, is still active), so the warning won't go stale as new settings are added or removed in future versions. The message points users at https://pnpm.io/settings rather than prescribing a single fix, since the new home depends on the key. Closes pnpm#11677.
9a53229 to
948e6e8
Compare
Previously the warning fired for every key under `pnpm` except `app`, which would surface false positives for third-party tooling that piggybacks on the `pnpm` namespace. Switch to an explicit denylist of the v10 settings that moved to pnpm-workspace.yaml, matching the PR's stated contract.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
config/reader/src/index.ts (1)
407-410: ⚡ Quick winConsider making the warning message more direct and consistent with the existing pattern.
The current warning directs users to the generic settings documentation, but the PR objectives and the comment at line 758 confirm that all these settings should move to
pnpm-workspace.yaml. The existing workspaces warning (line 405) uses a direct instruction:'Create a "pnpm-workspace.yaml" file instead.'A more direct message would improve clarity:
Suggested improvement
- warnings.push(`The "pnpm" field in package.json is no longer read by pnpm. The following keys were ignored: ${ignoredPnpmFieldKeys.map(k => `"pnpm.${k}"`).join(', ')}. See https://pnpm.io/settings for the new home of each setting.`) + warnings.push(`The "pnpm" field in package.json is no longer read by pnpm. Move the following settings to a "pnpm-workspace.yaml" file: ${ignoredPnpmFieldKeys.map(k => `"pnpm.${k}"`).join(', ')}.`)This matches the directness of the existing workspaces warning and aligns with the PR objectives example.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@config/reader/src/index.ts` around lines 407 - 410, The warning created when ignoredPnpmFieldKeys is non-empty (the block that calls getIgnoredPnpmFieldKeys and pushes into warnings via warnings.push) should be rewritten to match the direct style used for workspaces: replace the current message that points to https://pnpm.io/settings with a concise instruction telling the user to move those keys into a pnpm-workspace.yaml file (mentioning the specific ignored keys using ignoredPnpmFieldKeys.map(k => `"pnpm.${k}"`).join(', ') as before) so the message reads consistently with the workspaces warning and directs users to create a "pnpm-workspace.yaml" file instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@config/reader/src/index.ts`:
- Around line 407-410: The warning created when ignoredPnpmFieldKeys is
non-empty (the block that calls getIgnoredPnpmFieldKeys and pushes into warnings
via warnings.push) should be rewritten to match the direct style used for
workspaces: replace the current message that points to https://pnpm.io/settings
with a concise instruction telling the user to move those keys into a
pnpm-workspace.yaml file (mentioning the specific ignored keys using
ignoredPnpmFieldKeys.map(k => `"pnpm.${k}"`).join(', ') as before) so the
message reads consistently with the workspaces warning and directs users to
create a "pnpm-workspace.yaml" file instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0b132f5b-72c2-436b-809a-0c819f7e7660
📒 Files selected for processing (3)
config/reader/src/index.tsconfig/reader/test/fixtures/pkg-with-unknown-pnpm-field/package.jsonconfig/reader/test/index.ts
✅ Files skipped from review due to trivial changes (1)
- config/reader/test/fixtures/pkg-with-unknown-pnpm-field/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- config/reader/test/index.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Compile & Lint
- GitHub Check: Analyze (javascript)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use Standard Style with modifications: trailing commas are used, functions are preferred over classes, functions are declared after they are used (hoisting is relied upon).
Functions should have no more than two or three arguments. If a function needs more parameters, use a single options object instead.
Maintain import order: (1) Standard libraries, (2) External dependencies (sorted alphabetically), (3) Relative imports.
Files:
config/reader/src/index.ts
🧠 Learnings (1)
📚 Learning: 2026-05-14T09:04:00.133Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11622
File: resolving/npm-resolver/test/publishedBy.test.ts:350-354
Timestamp: 2026-05-14T09:04:00.133Z
Learning: In the pnpm/pnpm repository, ESLint is the authoritative style linter. Do not raise review findings for missing trailing commas in multiline function calls (e.g., `fs.writeFileSync(...)`) when this repo’s ESLint configuration does not report them and lint passes. Prefer deferring to the ESLint results for this specific trailing-comma rule rather than enforcing it manually in code review.
Applied to files:
config/reader/src/index.ts
🔇 Additional comments (2)
config/reader/src/index.ts (2)
757-779: LGTM!
781-787: LGTM!
zkochan
approved these changes
May 17, 2026
|
thanks |
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
4 participants
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
Closes #11677.
In pnpm v11, settings stopped being read from the
pnpmfield ofpackage.json(#10086). Settings likepnpm.overrides,pnpm.patchedDependencies,pnpm.packageExtensions,pnpm.allowedDeprecatedVersions, etc. are now expected to live inpnpm-workspace.yaml.Until now the old field was silently dropped, which is exactly the trap the issue reporter hit: an upgrade from v10 to v11 made their
pnpm.overridesandpnpm.patchedDependenciesstop taking effect without any signal that they had been ignored.This PR emits a warning when
package.jsonstill contains apnpmfield whose keys match the migrated settings, naming the offending keys and pointing at the new home — mirroring the existing "workspaces field in package.json" warning at the same site.Behavior
package.json:{ "pnpm": { "overrides": { "lodash": "^4.17.21" }, "patchedDependencies": { "is-odd": "patches/is-odd.patch" } } }Warning:
The warning only fires when keys under
pnpmmatch the set of migrated settings (overrides,patchedDependencies,packageExtensions,peerDependencyRules,allowedDeprecatedVersions,allowUnusedPatches,auditConfig,configDependencies,executionEnv,ignoredOptionalDependencies,neverBuiltDependencies,onlyBuiltDependencies,onlyBuiltDependenciesFile,requiredScripts,supportedArchitectures,updateConfig,allowBuilds). Unrelated keys placed underpnpmby third-party tooling do not trigger the warning.Test plan
config/reader/test/index.tsasserts the warning fires with the expected key list when a fixturepackage.jsoncontainspnpm.overridesandpnpm.patchedDependencies.do not return a warning if a package.json has workspaces field and there is a pnpm-workspace.yaml file) still pass — no false positives.warnings: []for the existingpkg-using-workspacesfixture.Local Jest could not run because of an unrelated
module is already linkedfailure underexperimental-vm-moduleson Node 22/24 (fails identically on a pristinemaincheckout) — happy to rerun once CI surfaces results.pacquet parity note
This change lives in TypeScript config-reading. Pacquet currently only ports
install; once it grows config-loading frompackage.json, the same warning belongs there. Flagging here perCLAUDE.mdparity guidance.Written by an agent (Claude Code, claude-opus-4-7).
Summary by CodeRabbit