From 4fa1091c80889bf594c10868a4e6717c19136a66 Mon Sep 17 00:00:00 2001 From: Brandon Cheng Date: Wed, 27 Jul 2022 05:27:41 -0400 Subject: [PATCH] feat: add experimental use-inline-specifiers-lockfile-format (#5091) * feat: add experimental use-inline-specifiers-lockfile-format * fix(lockfile-file): check importers key for shared lockfile format The `convertFromLockfileFileMutable` function reverts changes from `normalizeLockfile` when not using the shared lockfile format. - The non-shared lockfile format puts fields like `specifiers`, `dependencies`, `devDependencies`, `optionalDependencies`, and `dependenciesMeta` on the root of the lockfile. This is typically the case for a repo not using pnpm workspaces. - The shared lockfile format puts these under a `importers` block scoped by a path. The `use-inline-specifiers-lockfile-format` feature flag removes the `specifiers` block in favor of putting each specifier next to the resolved version within each `dependencies`, `devDependencies`, etc block. This means the `convertFromLockfileFileMutable` function can no longer check for `specifiers` to detect the whether the "shared" format is used. @zkochan suggested checking for `importers` instead, which should have the same effect. https://github.com/pnpm/pnpm/pull/5091#discussion_r929326835 * test(lockfile-file): add read & write test for useInlineSpecifiersFormat --- .changeset/fifty-rats-jog.md | 10 ++ packages/config/src/Config.ts | 3 + packages/config/src/index.ts | 2 + .../core/src/install/extendInstallOptions.ts | 2 + packages/core/src/install/index.ts | 7 +- .../experiments/InlineSpecifiersLockfile.ts | 38 ++++++ .../inlineSpecifiersLockfileConverters.ts | 114 ++++++++++++++++++ packages/lockfile-file/src/read.ts | 7 +- packages/lockfile-file/src/write.ts | 23 +++- .../test/__snapshots__/write.test.ts.snap | 15 +++ .../test/fixtures/7/pnpm-lock.yaml | 12 ++ packages/lockfile-file/test/read.test.ts | 27 +++++ packages/lockfile-file/test/write.test.ts | 36 ++++++ 13 files changed, 287 insertions(+), 9 deletions(-) create mode 100644 .changeset/fifty-rats-jog.md create mode 100644 packages/lockfile-file/src/experiments/InlineSpecifiersLockfile.ts create mode 100644 packages/lockfile-file/src/experiments/inlineSpecifiersLockfileConverters.ts create mode 100644 packages/lockfile-file/test/fixtures/7/pnpm-lock.yaml diff --git a/.changeset/fifty-rats-jog.md b/.changeset/fifty-rats-jog.md new file mode 100644 index 00000000000..0d7b42cdef9 --- /dev/null +++ b/.changeset/fifty-rats-jog.md @@ -0,0 +1,10 @@ +--- +"@pnpm/config": minor +"@pnpm/core": minor +"@pnpm/lockfile-file": minor +"pnpm": minor +--- + +Add experimental lockfile format that should merge conflict less in the `importers` section. Enabled by setting the `use-inline-specifiers-lockfile-format = true` feature flag in `.npmrc`. + +If this feature flag is committed to a repo, we recommend setting the minimum allowed version of pnpm to this release in the `package.json` `engines` field. Once this is set, older pnpm versions will throw on invalid lockfile versions. diff --git a/packages/config/src/Config.ts b/packages/config/src/Config.ts index e155d1c12b0..654fa9f1549 100644 --- a/packages/config/src/Config.ts +++ b/packages/config/src/Config.ts @@ -157,6 +157,9 @@ export interface Config { changedFilesIgnorePattern?: string[] rootProjectManifest?: ProjectManifest userConfig: Record + + // feature flags for experimental testing + useInlineSpecifiersLockfileFormat?: boolean // For https://github.com/pnpm/pnpm/issues/4725 } export interface ConfigWithDeprecatedSettings extends Config { diff --git a/packages/config/src/index.ts b/packages/config/src/index.ts index 2bd1f1a2926..0bbf968b08a 100644 --- a/packages/config/src/index.ts +++ b/packages/config/src/index.ts @@ -108,6 +108,7 @@ export const types = Object.assign({ stream: Boolean, 'strict-peer-dependencies': Boolean, 'use-beta-cli': Boolean, + 'use-inline-specifiers-lockfile-format': Boolean, 'use-node-version': String, 'use-running-store-server': Boolean, 'use-store-server': Boolean, @@ -222,6 +223,7 @@ export default async ( 'strict-peer-dependencies': true, 'unsafe-perm': npmDefaults['unsafe-perm'], 'use-beta-cli': false, + 'use-inline-specifiers-lockfile-format': false, userconfig: npmDefaults.userconfig, 'virtual-store-dir': 'node_modules/.pnpm', 'workspace-concurrency': 4, diff --git a/packages/core/src/install/extendInstallOptions.ts b/packages/core/src/install/extendInstallOptions.ts index 8d976fadec5..2d4e99fb994 100644 --- a/packages/core/src/install/extendInstallOptions.ts +++ b/packages/core/src/install/extendInstallOptions.ts @@ -28,6 +28,7 @@ export interface StrictInstallOptions { saveLockfile: boolean useGitBranchLockfile: boolean mergeGitBranchLockfiles: boolean + useInlineSpecifiersLockfileFormat: boolean linkWorkspacePackagesDepth: number lockfileOnly: boolean fixLockfile: boolean @@ -176,6 +177,7 @@ const defaults = async (opts: InstallOptions) => { useLockfile: true, saveLockfile: true, useGitBranchLockfile: false, + useInlineSpecifiersLockfileFormat: false, mergeGitBranchLockfiles: false, userAgent: `${packageManager.name}/${packageManager.version} npm/? node/${process.version} ${process.platform} ${process.arch}`, verifyStoreIntegrity: true, diff --git a/packages/core/src/install/index.ts b/packages/core/src/install/index.ts index 3af809f07e1..461e13ceea3 100644 --- a/packages/core/src/install/index.ts +++ b/packages/core/src/install/index.ts @@ -857,7 +857,12 @@ const _installInContext: InstallFunction = async (projects, ctx, opts) => { } const depsStateCache: DepsStateCache = {} - const lockfileOpts = { forceSharedFormat: opts.forceSharedLockfile, useGitBranchLockfile: opts.useGitBranchLockfile, mergeGitBranchLockfiles: opts.mergeGitBranchLockfiles } + const lockfileOpts = { + forceSharedFormat: opts.forceSharedLockfile, + useInlineSpecifiersFormat: opts.useInlineSpecifiersLockfileFormat, + useGitBranchLockfile: opts.useGitBranchLockfile, + mergeGitBranchLockfiles: opts.mergeGitBranchLockfiles, + } if (!opts.lockfileOnly && opts.enableModulesDir) { const result = await linkPackages( projects, diff --git a/packages/lockfile-file/src/experiments/InlineSpecifiersLockfile.ts b/packages/lockfile-file/src/experiments/InlineSpecifiersLockfile.ts new file mode 100644 index 00000000000..53aa145df62 --- /dev/null +++ b/packages/lockfile-file/src/experiments/InlineSpecifiersLockfile.ts @@ -0,0 +1,38 @@ +import type { Lockfile } from '@pnpm/lockfile-types' +import type { DependenciesMeta } from '@pnpm/types' + +export const INLINE_SPECIFIERS_FORMAT_LOCKFILE_VERSION_SUFFIX = '-inlineSpecifiers' + +/** + * Similar to the current Lockfile importers format (lockfile version 5.4 at + * time of writing), but specifiers are moved to each ResolvedDependencies block + * instead of being declared on its own dictionary. + * + * This is an experiment to reduce one flavor of merge conflicts in lockfiles. + * For more info: https://github.com/pnpm/pnpm/issues/4725. + */ +export interface InlineSpecifiersLockfile extends Omit { + lockfileVersion: string + importers: Record +} + +/** + * Similar to the current ProjectSnapshot interface, but omits the "specifiers" + * field in favor of inlining each specifier next to its version resolution in + * dependency blocks. + */ +export interface InlineSpecifiersProjectSnapshot { + dependencies?: InlineSpecifiersResolvedDependencies + devDependencies?: InlineSpecifiersResolvedDependencies + optionalDependencies?: InlineSpecifiersResolvedDependencies + dependenciesMeta?: DependenciesMeta +} + +export interface InlineSpecifiersResolvedDependencies { + [depName: string]: SpecifierAndResolution +} + +export interface SpecifierAndResolution { + specifier: string + version: string +} diff --git a/packages/lockfile-file/src/experiments/inlineSpecifiersLockfileConverters.ts b/packages/lockfile-file/src/experiments/inlineSpecifiersLockfileConverters.ts new file mode 100644 index 00000000000..e8fe95656fc --- /dev/null +++ b/packages/lockfile-file/src/experiments/inlineSpecifiersLockfileConverters.ts @@ -0,0 +1,114 @@ +import type { Lockfile, ProjectSnapshot, ResolvedDependencies } from '@pnpm/lockfile-types' +import { + INLINE_SPECIFIERS_FORMAT_LOCKFILE_VERSION_SUFFIX, + InlineSpecifiersLockfile, + InlineSpecifiersProjectSnapshot, + InlineSpecifiersResolvedDependencies, +} from './InlineSpecifiersLockfile' + +export function isExperimentalInlineSpecifiersFormat ( + lockfile: InlineSpecifiersLockfile | Lockfile +): lockfile is InlineSpecifiersLockfile { + const { lockfileVersion } = lockfile + return typeof lockfileVersion === 'string' && lockfileVersion.endsWith(INLINE_SPECIFIERS_FORMAT_LOCKFILE_VERSION_SUFFIX) +} + +export function convertToInlineSpecifiersFormat (lockfile: Lockfile): InlineSpecifiersLockfile { + return { + ...lockfile, + lockfileVersion: `${lockfile.lockfileVersion}${INLINE_SPECIFIERS_FORMAT_LOCKFILE_VERSION_SUFFIX}`, + importers: mapValues(lockfile.importers, convertProjectSnapshotToInlineSpecifiersFormat), + } +} + +export function revertFromInlineSpecifiersFormatIfNecessary (lockfile: Lockfile | InlineSpecifiersLockfile): Lockfile { + return isExperimentalInlineSpecifiersFormat(lockfile) + ? revertFromInlineSpecifiersFormat(lockfile) + : lockfile +} + +export function revertFromInlineSpecifiersFormat (lockfile: InlineSpecifiersLockfile): Lockfile { + const { lockfileVersion, importers, ...rest } = lockfile + + const originalVersionStr = lockfileVersion.replace(INLINE_SPECIFIERS_FORMAT_LOCKFILE_VERSION_SUFFIX, '') + const originalVersion = Number(originalVersionStr) + if (isNaN(originalVersion)) { + throw new Error(`Unable to revert lockfile from inline specifiers format. Invalid version parsed: ${originalVersionStr}`) + } + + return { + ...rest, + lockfileVersion: originalVersion, + importers: mapValues(importers, revertProjectSnapshot), + } +} + +function convertProjectSnapshotToInlineSpecifiersFormat ( + projectSnapshot: ProjectSnapshot +): InlineSpecifiersProjectSnapshot { + const { specifiers, ...rest } = projectSnapshot + const convertBlock = (block?: ResolvedDependencies) => + block != null + ? convertResolvedDependenciesToInlineSpecifiersFormat(block, { specifiers }) + : block + return { + ...rest, + dependencies: convertBlock(projectSnapshot.dependencies), + optionalDependencies: convertBlock(projectSnapshot.optionalDependencies), + devDependencies: convertBlock(projectSnapshot.devDependencies), + } +} + +function convertResolvedDependenciesToInlineSpecifiersFormat ( + resolvedDependencies: ResolvedDependencies, + { specifiers }: { specifiers: ResolvedDependencies} +): InlineSpecifiersResolvedDependencies { + return mapValues(resolvedDependencies, (version, depName) => ({ + specifier: specifiers[depName], + version, + })) +} + +function revertProjectSnapshot (from: InlineSpecifiersProjectSnapshot): ProjectSnapshot { + const specifiers: ResolvedDependencies = {} + + function moveSpecifiers (from: InlineSpecifiersResolvedDependencies): ResolvedDependencies { + const resolvedDependencies: ResolvedDependencies = {} + for (const [depName, { specifier, version }] of Object.entries(from)) { + const existingValue = specifiers[depName] + if (existingValue != null && existingValue !== specifier) { + throw new Error(`Project snapshot lists the same dependency more than once with conflicting versions: ${depName}`) + } + + specifiers[depName] = specifier + resolvedDependencies[depName] = version + } + return resolvedDependencies + } + + const dependencies = from.dependencies == null + ? from.dependencies + : moveSpecifiers(from.dependencies) + const devDependencies = from.devDependencies == null + ? from.devDependencies + : moveSpecifiers(from.devDependencies) + const optionalDependencies = from.optionalDependencies == null + ? from.optionalDependencies + : moveSpecifiers(from.optionalDependencies) + + return { + ...from, + specifiers, + dependencies, + devDependencies, + optionalDependencies, + } +} + +function mapValues (obj: Record, mapper: (val: T, key: string) => U): Record { + const result = {} + for (const [key, value] of Object.entries(obj)) { + result[key] = mapper(value, key) + } + return result +} diff --git a/packages/lockfile-file/src/read.ts b/packages/lockfile-file/src/read.ts index fca6a0f479b..9be964adeac 100644 --- a/packages/lockfile-file/src/read.ts +++ b/packages/lockfile-file/src/read.ts @@ -18,6 +18,7 @@ import logger from './logger' import { LockfileFile } from './write' import { getWantedLockfileName } from './lockfileName' import { getGitBranchLockfileNames } from './gitBranchLockfile' +import { revertFromInlineSpecifiersFormatIfNecessary } from './experiments/inlineSpecifiersLockfileConverters' export async function readCurrentLockfile ( virtualStoreDir: string, @@ -101,7 +102,7 @@ async function _read ( }) } if (lockfileFile) { - const lockfile = convertFromLockfileFileMutable(lockfileFile) + const lockfile = revertFromInlineSpecifiersFormatIfNecessary(convertFromLockfileFileMutable(lockfileFile)) const lockfileSemver = comverToSemver((lockfile.lockfileVersion ?? 0).toString()) /* eslint-enable @typescript-eslint/dot-notation */ if (typeof opts.wantedVersion !== 'number' || semver.major(lockfileSemver) === semver.major(comverToSemver(opts.wantedVersion.toString()))) { @@ -225,10 +226,10 @@ async function _readGitBranchLockfiles ( * Reverts changes from the "forceSharedFormat" write option if necessary. */ function convertFromLockfileFileMutable (lockfileFile: LockfileFile): Lockfile { - if (typeof lockfileFile?.['specifiers'] !== 'undefined') { + if (typeof lockfileFile?.['importers'] === 'undefined') { lockfileFile.importers = { '.': { - specifiers: lockfileFile['specifiers'], + specifiers: lockfileFile['specifiers'] ?? {}, dependenciesMeta: lockfileFile['dependenciesMeta'], }, } diff --git a/packages/lockfile-file/src/write.ts b/packages/lockfile-file/src/write.ts index bbca62cb19e..e82649bce08 100644 --- a/packages/lockfile-file/src/write.ts +++ b/packages/lockfile-file/src/write.ts @@ -11,6 +11,7 @@ import writeFileAtomicCB from 'write-file-atomic' import logger from './logger' import { sortLockfileKeys } from './sortLockfileKeys' import { getWantedLockfileName } from './lockfileName' +import { convertToInlineSpecifiersFormat } from './experiments/inlineSpecifiersLockfileConverters' async function writeFileAtomic (filename: string, data: string) { return new Promise((resolve, reject) => writeFileAtomicCB(filename, data, {}, (err?: Error) => (err != null) ? reject(err) : resolve())) @@ -29,6 +30,7 @@ export async function writeWantedLockfile ( wantedLockfile: Lockfile, opts?: { forceSharedFormat?: boolean + useInlineSpecifiersFormat?: boolean useGitBranchLockfile?: boolean mergeGitBranchLockfiles?: boolean } @@ -48,13 +50,16 @@ export async function writeCurrentLockfile ( return writeLockfile('lock.yaml', virtualStoreDir, currentLockfile, opts) } +interface LockfileFormatOptions { + forceSharedFormat?: boolean + useInlineSpecifiersFormat?: boolean +} + async function writeLockfile ( lockfileFilename: string, pkgPath: string, wantedLockfile: Lockfile, - opts?: { - forceSharedFormat?: boolean - } + opts?: LockfileFormatOptions ) { const lockfilePath = path.join(pkgPath, lockfileFilename) @@ -63,7 +68,11 @@ async function writeLockfile ( return rimraf(lockfilePath) } - const yamlDoc = yamlStringify(wantedLockfile, opts?.forceSharedFormat === true) + const lockfileToStringify = (opts?.useInlineSpecifiersFormat ?? false) + ? convertToInlineSpecifiersFormat(wantedLockfile) as unknown as Lockfile + : wantedLockfile + + const yamlDoc = yamlStringify(lockfileToStringify, opts?.forceSharedFormat === true) return writeFileAtomic(lockfilePath, yamlDoc) } @@ -145,6 +154,7 @@ export function normalizeLockfile (lockfile: Lockfile, forceSharedFormat: boolea export default async function writeLockfiles ( opts: { forceSharedFormat?: boolean + useInlineSpecifiersFormat?: boolean wantedLockfile: Lockfile wantedLockfileDir: string currentLockfile: Lockfile @@ -167,7 +177,10 @@ export default async function writeLockfiles ( } const forceSharedFormat = opts?.forceSharedFormat === true - const yamlDoc = yamlStringify(opts.wantedLockfile, forceSharedFormat) + const wantedLockfileToStringify = (opts.useInlineSpecifiersFormat ?? false) + ? convertToInlineSpecifiersFormat(opts.wantedLockfile) as unknown as Lockfile + : opts.wantedLockfile + const yamlDoc = yamlStringify(wantedLockfileToStringify, forceSharedFormat) // in most cases the `pnpm-lock.yaml` and `node_modules/.pnpm-lock.yaml` are equal // in those cases the YAML document can be stringified only once for both files diff --git a/packages/lockfile-file/test/__snapshots__/write.test.ts.snap b/packages/lockfile-file/test/__snapshots__/write.test.ts.snap index a2bb8ce94e4..765c35e03dd 100644 --- a/packages/lockfile-file/test/__snapshots__/write.test.ts.snap +++ b/packages/lockfile-file/test/__snapshots__/write.test.ts.snap @@ -29,3 +29,18 @@ packages: resolution: {integrity: sha1-ChbBDewTLAqLCzb793Fo5VDvg/g=} " `; + +exports[`writeLockfiles() when useInlineSpecifiersFormat 1`] = ` +"lockfileVersion: 5.4-inlineSpecifiers + +dependencies: + foo: + specifier: ^1.0.0 + version: 1.0.0 + +packages: + + /foo/1.0.0: + resolution: {integrity: sha1-ChbBDewTLAqLCzb793Fo5VDvg/g=} +" +`; diff --git a/packages/lockfile-file/test/fixtures/7/pnpm-lock.yaml b/packages/lockfile-file/test/fixtures/7/pnpm-lock.yaml new file mode 100644 index 00000000000..7b8f360e1dc --- /dev/null +++ b/packages/lockfile-file/test/fixtures/7/pnpm-lock.yaml @@ -0,0 +1,12 @@ +lockfileVersion: 5.3-inlineSpecifiers + +specifiers: {} + +dependencies: + is-positive: + specifier: '^1.0.0' + version: '1.0.0' + +packages: + /is-positive/1.0.0: + resolution: {integrity: 'sha1-ChbBDewTLAqLCzb793Fo5VDvg/g='} diff --git a/packages/lockfile-file/test/read.test.ts b/packages/lockfile-file/test/read.test.ts index 1becc08c588..05596cec4b4 100644 --- a/packages/lockfile-file/test/read.test.ts +++ b/packages/lockfile-file/test/read.test.ts @@ -261,3 +261,30 @@ test('readWantedLockfile() when useGitBranchLockfile and mergeGitBranchLockfiles }, }) }) + +test('readWantedLockfile() with inlineSpecifiersFormat', async () => { + const wantedLockfile = { + importers: { + '.': { + dependencies: { + 'is-positive': '1.0.0', + }, + specifiers: { + 'is-positive': '^1.0.0', + }, + }, + }, + packages: { + '/is-positive/1.0.0': { + resolution: { + integrity: 'sha1-ChbBDewTLAqLCzb793Fo5VDvg/g=', + }, + }, + }, + registry: 'https://registry.npmjs.org', + } + + const lockfile = await readWantedLockfile(path.join('fixtures', '7'), { ignoreIncompatible: false }) + expect(lockfile?.importers).toEqual(wantedLockfile.importers) + expect(lockfile?.packages).toEqual(wantedLockfile.packages) +}) diff --git a/packages/lockfile-file/test/write.test.ts b/packages/lockfile-file/test/write.test.ts index 69a74d8d216..63fd0474d70 100644 --- a/packages/lockfile-file/test/write.test.ts +++ b/packages/lockfile-file/test/write.test.ts @@ -231,3 +231,39 @@ test('writeLockfiles() when useGitBranchLockfile', async () => { expect(fs.existsSync(path.join(projectPath, WANTED_LOCKFILE))).toBeFalsy() expect(fs.existsSync(path.join(projectPath, `pnpm-lock.${branchName}.yaml`))).toBeTruthy() }) + +test('writeLockfiles() when useInlineSpecifiersFormat', async () => { + const projectPath = tempy.directory() + const wantedLockfile = { + importers: { + '.': { + dependencies: { + foo: '1.0.0', + }, + specifiers: { + foo: '^1.0.0', + }, + }, + }, + lockfileVersion: LOCKFILE_VERSION, + packages: { + '/foo/1.0.0': { + resolution: { + integrity: 'sha1-ChbBDewTLAqLCzb793Fo5VDvg/g=', + }, + }, + }, + } + + await writeLockfiles({ + currentLockfile: wantedLockfile, + currentLockfileDir: projectPath, + wantedLockfile, + wantedLockfileDir: projectPath, + useInlineSpecifiersFormat: true, + }) + + expect(await readCurrentLockfile(projectPath, { ignoreIncompatible: false })).toEqual(wantedLockfile) + expect(await readWantedLockfile(projectPath, { ignoreIncompatible: false })).toEqual(wantedLockfile) + expect(fs.readFileSync(path.join(projectPath, WANTED_LOCKFILE), 'utf8')).toMatchSnapshot() +})