refactor: merge env lockfile into pnpm-lock.yaml#10964
Merged
Conversation
81da7f6 to
74b2726
Compare
Instead of a separate pnpm-lock.env.yaml file, the env lockfile (configDependencies and packageManagerDependencies) is now stored as the first YAML document in pnpm-lock.yaml, separated by `---`. The combined file starts with `---\n` when an env document is present, allowing pnpm to check just the first 4 bytes to know whether the file contains an env document. Reading uses streaming I/O that stops as soon as the document separator is found, avoiding parsing of the full lockfile. Writing preserves both documents: when the env lockfile is updated the main lockfile portion is kept, and vice versa. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
74b2726 to
c0dcdee
Compare
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Remove readEnvYamlPrefix by reusing streamReadFirstYamlDocument. Use two-phase streaming (verify prefix, then find separator) to avoid redundant startsWith checks. Strip BOM on first chunk for correct prefix detection. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
extractMainDocument now returns empty string when there is no second document separator, instead of returning the env content as if it were the main lockfile. Also adds comments explaining the re-read approach in write.ts. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…-trips Add yamlDocuments.test.ts with tests for streamReadFirstYamlDocument (happy path, no prefix, ENOENT, no separator, BOM, empty file, multiline) and extractMainDocument (no prefix, no separator, happy path). Add write.test.ts tests for env document preservation through writeLockfiles/writeWantedLockfile and readWantedLockfile with combined files. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors pnpm’s “env lockfile” (for configDependencies and packageManagerDependencies) to live inside pnpm-lock.yaml as a first YAML document, replacing the separate pnpm-lock.env.yaml file. This aligns env-lockfile persistence with the main lockfile while aiming to keep reads efficient via streaming.
Changes:
- Store env-lockfile data as the first YAML document in
pnpm-lock.yamland add helpers to stream-read/extract YAML documents. - Update lockfile read/write paths to preserve the env document when writing the main lockfile, and to skip the env document when reading the main lockfile.
- Update tests, constants, error messages, and helper utilities to reference
pnpm-lock.yamlinstead ofpnpm-lock.env.yaml.
Reviewed changes
Copilot reviewed 22 out of 23 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/plugin-commands-self-updater/test/selfUpdate.test.ts | Updates self-updater tests to look at pnpm-lock.yaml. |
| tools/plugin-commands-self-updater/src/selfUpdate.ts | Updates comment to reflect new env-lockfile storage location. |
| pnpm/test/switchingVersions.test.ts | Updates version-switching tests to expect pnpm-lock.yaml. |
| pnpm/test/configurationalDependencies.test.ts | Adjusts configurational dependency tests around env-lockfile expectations. |
| pnpm/src/switchCliVersion.ts | Updates error message to refer to pnpm-lock.yaml. |
| pnpm-lock.yaml | Adds js-yaml and @types/js-yaml to the workspace lockfile. |
| packages/types/src/package.ts | Updates documentation comments to reference pnpm-lock.yaml. |
| packages/constants/src/index.ts | Removes ENV_LOCKFILE constant export. |
| lockfile/fs/test/yamlDocuments.test.ts | Adds unit tests for streaming first-doc reads and main-doc extraction. |
| lockfile/fs/test/write.test.ts | Adds tests ensuring env document is preserved/skipped correctly. |
| lockfile/fs/src/yamlDocuments.ts | Introduces helpers for streaming first YAML doc and extracting main doc. |
| lockfile/fs/src/write.ts | Preserves env document when writing pnpm-lock.yaml. |
| lockfile/fs/src/read.ts | Skips env document before parsing the main lockfile document. |
| lockfile/fs/src/envLockfile.ts | Reads/writes env lockfile as the first document within pnpm-lock.yaml. |
| config/deps-installer/test/installConfigDeps.ts | Updates env-lockfile assertions to read from combined lockfile. |
| config/deps-installer/src/resolvePackageManagerIntegrities.ts | Updates doc comment to reflect new storage location. |
| config/deps-installer/src/migrateConfigDeps.ts | Updates migration messaging to reference pnpm-lock.yaml. |
| config/deps-installer/src/installConfigDeps.ts | Updates corruption error messages to reference pnpm-lock.yaml. |
| utils/assert-project/src/index.ts | Updates lockfile-reading helper to skip env document before parsing. |
| utils/assert-project/package.json | Adds js-yaml (+ types) dependency to support the updated parsing approach. |
| .changeset/dev-engines-package-manager.md | Updates changeset text to reference pnpm-lock.yaml. |
| .changeset/config-deps-lockfile.md | Updates changeset text to describe env doc stored in pnpm-lock.yaml. |
| .changeset/audit-env-lockfile.md | Updates changeset text to reference pnpm-lock.yaml. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
+91
to
+98
| // Skip the env lockfile document if present (first document in combined format) | ||
| lockfileRawContent = extractMainDocument(lockfileRawContent) | ||
| if (!lockfileRawContent.trim()) { | ||
| return { | ||
| lockfile: null, | ||
| hadConflicts: false, | ||
| } | ||
| } |
|
|
||
| if (!envLockfile) { | ||
| throw new PnpmError('NO_PKG_MANAGER_INTEGRITY', `The packageManager dependency ${pmVersion} was not found in the pnpm-lock.env.yaml`) | ||
| throw new PnpmError('NO_PKG_MANAGER_INTEGRITY', `The packageManager dependency ${pmVersion} was not found in pnpm-lock.yaml`) |
| @@ -134,14 +120,6 @@ | |||
|
|
|||
| // pnpm 10.32.0 supports configDependencies and should have installed them | |||
| expect(fs.existsSync('node_modules/.pnpm-config/@pnpm.e2e/has-patch-for-foo')).toBeTruthy() | |||
Comment on lines
33
to
49
| const parsed = yaml.load(rawContent) | ||
| if (parsed == null || typeof parsed !== 'object') { | ||
| throw new PnpmError('INVALID_ENV_LOCKFILE', `Invalid env lockfile at ${lockfilePath}: expected a YAML object`) | ||
| return null | ||
| } | ||
| const lockfile = parsed as Record<string, unknown> | ||
| if (typeof lockfile.lockfileVersion !== 'string') { | ||
| throw new PnpmError('INVALID_ENV_LOCKFILE', `Invalid env lockfile at ${lockfilePath}: missing or non-string "lockfileVersion"`) | ||
| return null | ||
| } | ||
| if (lockfile.importers == null || typeof lockfile.importers !== 'object') { | ||
| throw new PnpmError('INVALID_ENV_LOCKFILE', `Invalid env lockfile at ${lockfilePath}: missing or invalid "importers"`) | ||
| return null | ||
| } | ||
| if (lockfile.packages == null || typeof lockfile.packages !== 'object') { | ||
| throw new PnpmError('INVALID_ENV_LOCKFILE', `Invalid env lockfile at ${lockfilePath}: missing or invalid "packages"`) | ||
| return null | ||
| } | ||
| if (lockfile.snapshots == null || typeof lockfile.snapshots !== 'object') { | ||
| throw new PnpmError('INVALID_ENV_LOCKFILE', `Invalid env lockfile at ${lockfilePath}: missing or invalid "snapshots"`) | ||
| return null | ||
| } |
Comment on lines
+64
to
+69
| // Read existing main lockfile document to preserve it | ||
| let mainDoc = '' | ||
| try { | ||
| const existing = await fs.readFile(lockfilePath, 'utf8') | ||
| mainDoc = extractMainDocument(existing) | ||
| } catch (err: unknown) { |
Comment on lines
+338
to
340
| const lockfile = fs.readFileSync(path.join(opts.dir, 'pnpm-lock.yaml'), 'utf8') | ||
| expect(lockfile).toContain('9.0.0') | ||
| }) |
Comment on lines
80
to
102
| test('config deps are not installed before switching to a different pnpm version', async () => { | ||
| prepare() | ||
| const pnpmHome = path.resolve('pnpm') | ||
| const env = { PNPM_HOME: pnpmHome } | ||
|
|
||
| // First, add config dep to create the env lockfile (clean specifier format) | ||
| await execPnpm(['add', '@pnpm.e2e/has-patch-for-foo@1.0.0', '--config'], { env }) | ||
|
|
||
| // Remove node_modules so we can check if config deps get re-installed | ||
| fs.rmSync('node_modules', { recursive: true }) | ||
|
|
||
| // Switch to pnpm 9.3.0, which doesn't know about configDependencies. | ||
| // If the current pnpm installed config deps before switching, the directory would exist. | ||
| writeJsonFileSync('package.json', { | ||
| packageManager: 'pnpm@9.3.0', | ||
| }) | ||
|
|
||
| execPnpmSync(['install'], { env, stdio: 'pipe' }) | ||
|
|
||
| // Config deps should NOT be installed — pnpm 9.3.0 doesn't support them, | ||
| // and the current pnpm should not have installed them before switching. | ||
| expect(fs.existsSync('node_modules/.pnpm-config/@pnpm.e2e/has-patch-for-foo')).toBeFalsy() | ||
|
|
||
| // The env lockfile should have packageManagerDependencies from the version switch | ||
| const envLockfile = await readEnvLockfile(process.cwd()) | ||
| expect(envLockfile).not.toBeNull() | ||
| expect(envLockfile!.importers['.'].configDependencies['@pnpm.e2e/has-patch-for-foo']).toBeDefined() | ||
| expect(envLockfile!.importers['.'].packageManagerDependencies).toBeDefined() | ||
| expect(envLockfile!.importers['.'].packageManagerDependencies!['pnpm']).toStrictEqual({ | ||
| specifier: '9.3.0', | ||
| version: '9.3.0', | ||
| }) | ||
| expect(envLockfile!.importers['.'].packageManagerDependencies!['@pnpm/exe']).toStrictEqual({ | ||
| specifier: '9.3.0', | ||
| version: '9.3.0', | ||
| }) | ||
| }) |
…icating logic Address Copilot review feedback: - Export extractMainDocument from @pnpm/lockfile.fs - Use it in assert-project instead of inline separator logic - Handle empty content after extraction (env-only lockfile) - Use temporaryDirectory() for non-existent file test path Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…in assert-project Revert adding @pnpm/lockfile.fs as dependency of assert-project since it creates a circular reference through lockfile/fs test dependencies. Keep the inline logic with a startsWith guard and empty content check. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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-lock.env.yamlfile, the env lockfile (configDependencies and packageManagerDependencies) is now stored as the first YAML document inpnpm-lock.yaml, separated by---ENV_LOCKFILEconstant and updated all string referencesTest plan
@pnpm/lockfile.fsunit tests pass (25/25)@pnpm/config.deps-installertestspnpmintegration tests (switchingVersions, configurationalDependencies)plugin-commands-self-updatertests🤖 Generated with Claude Code