From 9083673db8003db7e2bdae1d6182fbb0ea67c18f Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Sat, 10 Dec 2022 16:25:36 +0100 Subject: [PATCH 01/14] cli: slight lockfile lib refactor Signed-off-by: Patrik Oldsberg --- packages/cli/src/commands/versions/bump.ts | 2 +- .../cli/src/lib/versioning/Lockfile.test.ts | 4 +- packages/cli/src/lib/versioning/Lockfile.ts | 46 +++++++------------ 3 files changed, 20 insertions(+), 32 deletions(-) diff --git a/packages/cli/src/commands/versions/bump.ts b/packages/cli/src/commands/versions/bump.ts index 37eb858dcf..45b966a52d 100644 --- a/packages/cli/src/commands/versions/bump.ts +++ b/packages/cli/src/commands/versions/bump.ts @@ -201,7 +201,7 @@ export default async (opts: OptionValues) => { lockfile.remove(name, range); } } - await lockfile.save(); + await lockfile.save(lockfilePath); } const breakingUpdates = new Map(); diff --git a/packages/cli/src/lib/versioning/Lockfile.test.ts b/packages/cli/src/lib/versioning/Lockfile.test.ts index e663ec4a93..e2842b4c95 100644 --- a/packages/cli/src/lib/versioning/Lockfile.test.ts +++ b/packages/cli/src/lib/versioning/Lockfile.test.ts @@ -111,7 +111,7 @@ describe('Lockfile', () => { expect(lockfile.toString()).toBe(mockADedup); await expect(fs.readFile('/yarn.lock', 'utf8')).resolves.toBe(mockA); - await expect(lockfile.save()).resolves.toBeUndefined(); + await expect(lockfile.save('/yarn.lock')).resolves.toBeUndefined(); await expect(fs.readFile('/yarn.lock', 'utf8')).resolves.toBe(mockADedup); }); @@ -268,7 +268,7 @@ describe('New Lockfile', () => { expect(lockfile.toString()).toBe(mockANewDedup); await expect(fs.readFile('/yarn.lock', 'utf8')).resolves.toBe(mockANew); - await expect(lockfile.save()).resolves.toBeUndefined(); + await expect(lockfile.save('/yarn.lock')).resolves.toBeUndefined(); await expect(fs.readFile('/yarn.lock', 'utf8')).resolves.toBe( mockANewDedup, ); diff --git a/packages/cli/src/lib/versioning/Lockfile.ts b/packages/cli/src/lib/versioning/Lockfile.ts index 0559ca86a1..faafb465a4 100644 --- a/packages/cli/src/lib/versioning/Lockfile.ts +++ b/packages/cli/src/lib/versioning/Lockfile.ts @@ -65,20 +65,6 @@ type AnalyzeResult = { newRanges: AnalyzeResultNewRange[]; }; -function parseLockfile(lockfileContents: string) { - try { - return { - object: parseSyml(lockfileContents), - type: 'success', - }; - } catch (err) { - return { - object: null, - type: err, - }; - } -} - // the new yarn header is handled out of band of the parsing // https://github.com/yarnpkg/berry/blob/0c5974f193a9397630e9aee2b3876cca62611149/packages/yarnpkg-core/sources/Project.ts#L1741-L1746 const NEW_HEADER = `${[ @@ -86,11 +72,6 @@ const NEW_HEADER = `${[ `# Manual changes might be lost - proceed with caution!\n`, ].join(``)}\n`; -function stringifyLockfile(data: LockfileData, legacy: boolean) { - return legacy - ? legacyStringifyLockfile(data) - : NEW_HEADER + stringifySyml(data); -} // taken from yarn parser package // https://github.com/yarnpkg/berry/blob/0c5974f193a9397630e9aee2b3876cca62611149/packages/yarnpkg-parsers/sources/syml.ts#L136 const LEGACY_REGEX = /^(#.*(\r?\n))*?#\s+yarn\s+lockfile\s+v1\r?\n/i; @@ -111,13 +92,19 @@ const SPECIAL_OBJECT_KEYS = [ export class Lockfile { static async load(path: string) { const lockfileContents = await fs.readFile(path, 'utf8'); - const legacy = LEGACY_REGEX.test(lockfileContents); - const lockfile = parseLockfile(lockfileContents); - if (lockfile.type !== 'success') { - throw new Error(`Failed yarn.lock parse with ${lockfile.type}`); + return Lockfile.parse(lockfileContents); + } + + static parse(content: string) { + const legacy = LEGACY_REGEX.test(content); + + let data: LockfileData; + try { + data = parseSyml(content); + } catch (err) { + throw new Error(`Failed yarn.lock parse, ${err}`); } - const data = lockfile.object as LockfileData; const packages = new Map(); for (const [key, value] of Object.entries(data)) { @@ -144,11 +131,10 @@ export class Lockfile { } } - return new Lockfile(path, packages, data, legacy); + return new Lockfile(packages, data, legacy); } private constructor( - private readonly path: string, private readonly packages: Map, private readonly data: LockfileData, private readonly legacy: boolean = false, @@ -340,11 +326,13 @@ export class Lockfile { } } - async save() { - await fs.writeFile(this.path, this.toString(), 'utf8'); + async save(path: string) { + await fs.writeFile(path, this.toString(), 'utf8'); } toString() { - return stringifyLockfile(this.data, this.legacy); + return this.legacy + ? legacyStringifyLockfile(this.data) + : NEW_HEADER + stringifySyml(this.data); } } From d2ad8ca3af5fc6d4f8c5306e76c0bb930841d4c2 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Mon, 12 Dec 2022 18:56:56 +0100 Subject: [PATCH 02/14] cli: add Lockfile.diff Signed-off-by: Patrik Oldsberg --- .../cli/src/lib/versioning/Lockfile.test.ts | 236 +++++++++++++++++- packages/cli/src/lib/versioning/Lockfile.ts | 61 ++++- 2 files changed, 288 insertions(+), 9 deletions(-) diff --git a/packages/cli/src/lib/versioning/Lockfile.test.ts b/packages/cli/src/lib/versioning/Lockfile.test.ts index e2842b4c95..8209ef1f23 100644 --- a/packages/cli/src/lib/versioning/Lockfile.test.ts +++ b/packages/cli/src/lib/versioning/Lockfile.test.ts @@ -78,10 +78,12 @@ describe('Lockfile', () => { }); const lockfile = await Lockfile.load('/yarn.lock'); - expect(lockfile.get('a')).toEqual([{ range: '^1', version: '1.0.1' }]); + expect(lockfile.get('a')).toEqual([ + { range: '^1', version: '1.0.1', dataKey: 'a@^1' }, + ]); expect(lockfile.get('b')).toEqual([ - { range: '2.0.x', version: '2.0.1' }, - { range: '^2', version: '2.0.0' }, + { range: '2.0.x', version: '2.0.1', dataKey: 'b@2.0.x' }, + { range: '^2', version: '2.0.0', dataKey: 'b@^2' }, ]); expect(lockfile.toString()).toBe(mockA); }); @@ -234,11 +236,13 @@ describe('New Lockfile', () => { }); const lockfile = await Lockfile.load('/yarn.lock'); - expect(lockfile.get('a')).toEqual([{ range: '^1', version: '1.0.1' }]); + expect(lockfile.get('a')).toEqual([ + { range: '^1', version: '1.0.1', dataKey: 'a@^1' }, + ]); expect(lockfile.get('b')).toEqual([ - { range: '2.0.x', version: '2.0.1' }, - { range: '^2.0.1', version: '2.0.1' }, - { range: '^2', version: '2.0.0' }, + { range: '2.0.x', version: '2.0.1', dataKey: 'b@2.0.x, b@^2.0.1' }, + { range: '^2.0.1', version: '2.0.1', dataKey: 'b@2.0.x, b@^2.0.1' }, + { range: '^2', version: '2.0.0', dataKey: 'b@^2' }, ]); expect(lockfile.toString()).toBe(mockANew); }); @@ -315,4 +319,222 @@ describe('New Lockfile', () => { mockANewLocalDedup, ); }); + + describe('diff', () => { + const lockfileLegacyA = Lockfile.parse(`${HEADER} +a@^1: + version "1.0.1" + resolved "https://my-registry/a-1.0.01.tgz#abc123" + integrity sha512-xyz + dependencies: + b "^2" + +b@3: + version "3.0.1" + integrity sha512-abc1 + +b@2.0.x: + version "2.0.1" + integrity sha512-abc2 + +b@^2: + version "2.0.0" + integrity sha512-abc3 + +c@^1: + version "1.0.1" + integrity x +`); + + const lockfileLegacyB = Lockfile.parse(`${HEADER} +a@^1: + version "1.0.1" + resolved "https://my-registry/a-1.0.01.tgz#abc123" + integrity sha512-xyz-other + dependencies: + b "^2" + +b@2.0.x, b@^2: + version "2.0.0" + integrity sha512-abc3 + +b@4: + version "4.0.0" + integrity sha512-abc + +d@^1: + version "1.0.1" + integrity x +`); + + const lockfileModernA = Lockfile.parse(`${HEADER} +"a@^1": + version "1.0.1" + resolved "https://my-registry/a-1.0.01.tgz#abc123" + checksum sha512-xyz + dependencies: + b "^2" + +"b@3": + version "3.0.1" + checksum sha512-abc1 + +"b@2.0.x": + version "2.0.1" + checksum sha512-abc2 + +"b@^2": + version "2.0.0" + checksum sha512-abc3 + +"c@^1": + version "1.0.1" + checksum x +`); + + const lockfileModernB = Lockfile.parse(`${HEADER} +"a@^1": + version "1.0.1" + resolution "a@npm:1.0.1" + checksum sha512-xyz-other + dependencies: + b "^2" + +"b@2.0.x, b@^2": + version "2.0.0" + checksum sha512-abc3 + +"b@4": + version "4.0.0" + checksum sha512-abc + +"d@^1": + version "1.0.1" + checksum x +`); + + it('should diff two legacy lockfiles', async () => { + expect(lockfileLegacyA.diff(lockfileLegacyB)).toEqual({ + added: [ + { name: 'b', range: '4' }, + { name: 'd', range: '^1' }, + ], + changed: [ + { name: 'a', range: '^1' }, + { name: 'b', range: '2.0.x' }, + ], + removed: [ + { name: 'b', range: '3' }, + { name: 'c', range: '^1' }, + ], + }); + expect(lockfileLegacyB.diff(lockfileLegacyA)).toEqual({ + added: [ + { name: 'b', range: '3' }, + { name: 'c', range: '^1' }, + ], + changed: [ + { name: 'a', range: '^1' }, + { name: 'b', range: '2.0.x' }, + ], + removed: [ + { name: 'b', range: '4' }, + { name: 'd', range: '^1' }, + ], + }); + }); + + it('should diff two modern lockfiles', async () => { + expect(lockfileModernA.diff(lockfileModernB)).toEqual({ + added: [ + { name: 'b', range: '4' }, + { name: 'd', range: '^1' }, + ], + changed: [ + { name: 'a', range: '^1' }, + { name: 'b', range: '2.0.x' }, + ], + removed: [ + { name: 'b', range: '3' }, + { name: 'c', range: '^1' }, + ], + }); + expect(lockfileModernB.diff(lockfileModernA)).toEqual({ + added: [ + { name: 'b', range: '3' }, + { name: 'c', range: '^1' }, + ], + changed: [ + { name: 'a', range: '^1' }, + { name: 'b', range: '2.0.x' }, + ], + removed: [ + { name: 'b', range: '4' }, + { name: 'd', range: '^1' }, + ], + }); + }); + + it('should diff legacy and modern lockfiles', async () => { + expect(lockfileLegacyA.diff(lockfileModernB)).toEqual({ + added: [ + { name: 'b', range: '4' }, + { name: 'd', range: '^1' }, + ], + changed: [ + { name: 'a', range: '^1' }, + { name: 'b', range: '2.0.x' }, + ], + removed: [ + { name: 'b', range: '3' }, + { name: 'c', range: '^1' }, + ], + }); + expect(lockfileLegacyB.diff(lockfileModernA)).toEqual({ + added: [ + { name: 'b', range: '3' }, + { name: 'c', range: '^1' }, + ], + changed: [ + { name: 'a', range: '^1' }, + { name: 'b', range: '2.0.x' }, + ], + removed: [ + { name: 'b', range: '4' }, + { name: 'd', range: '^1' }, + ], + }); + }); + + it('should diff modern and legacy lockfiles', async () => { + expect(lockfileModernA.diff(lockfileLegacyB)).toEqual({ + added: [ + { name: 'b', range: '4' }, + { name: 'd', range: '^1' }, + ], + changed: [ + { name: 'a', range: '^1' }, + { name: 'b', range: '2.0.x' }, + ], + removed: [ + { name: 'b', range: '3' }, + { name: 'c', range: '^1' }, + ], + }); + expect(lockfileModernB.diff(lockfileLegacyA)).toEqual({ + added: [ + { name: 'b', range: '3' }, + { name: 'c', range: '^1' }, + ], + changed: [ + { name: 'a', range: '^1' }, + { name: 'b', range: '2.0.x' }, + ], + removed: [ + { name: 'b', range: '4' }, + { name: 'd', range: '^1' }, + ], + }); + }); + }); }); diff --git a/packages/cli/src/lib/versioning/Lockfile.ts b/packages/cli/src/lib/versioning/Lockfile.ts index faafb465a4..d156c9498d 100644 --- a/packages/cli/src/lib/versioning/Lockfile.ts +++ b/packages/cli/src/lib/versioning/Lockfile.ts @@ -26,7 +26,8 @@ type LockfileData = { [entry: string]: { version: string; resolved?: string; - integrity?: string; + integrity?: string /* old */; + checksum?: string /* new */; dependencies?: { [name: string]: string }; }; }; @@ -34,6 +35,7 @@ type LockfileData = { type LockfileQueryEntry = { range: string; version: string; + dataKey: string; }; /** Entries that have an invalid version range, for example an npm tag */ @@ -127,7 +129,7 @@ export class Lockfile { if (range.startsWith('npm:')) { range = range.slice('npm:'.length); } - queries.push({ range, version: value.version }); + queries.push({ range, version: value.version, dataKey: key }); } } @@ -326,6 +328,61 @@ export class Lockfile { } } + diff(newLockfile: Lockfile) { + const diff = { + added: new Array<{ name: string; range: string }>(), + removed: new Array<{ name: string; range: string }>(), + changed: new Array<{ name: string; range: string }>(), + }; + + // Keeps track of packages that only exist in the old lockfile + const remainingOldNames = new Set(this.packages.keys()); + + for (const [name, newQueries] of newLockfile.packages) { + remainingOldNames.delete(name); + + const oldQueries = this.packages.get(name); + // If the packages doesn't exist in the old lockfile, add all entries + if (!oldQueries) { + diff.added.push(...newQueries.map(q => ({ name, range: q.range }))); + continue; + } + + const remainingOldRanges = new Set(oldQueries.map(q => q.range)); + + for (const newQuery of newQueries) { + remainingOldRanges.delete(newQuery.range); + + const oldQuery = oldQueries.find(q => q.range === newQuery.range); + if (!oldQuery) { + diff.added.push({ name, range: newQuery.range }); + continue; + } + + const newPkg = newLockfile.data[newQuery.dataKey]; + const oldPkg = this.data[oldQuery.dataKey]; + if (newPkg && oldPkg) { + const oldCheck = oldPkg.integrity || oldPkg.checksum; + const newCheck = newPkg.integrity || newPkg.checksum; + if (!oldCheck || !newCheck || oldCheck !== newCheck) { + diff.changed.push({ name, range: newQuery.range }); + } + } + } + + for (const oldRange of remainingOldRanges) { + diff.removed.push({ name, range: oldRange }); + } + } + + for (const name of remainingOldNames) { + const queries = this.packages.get(name) ?? []; + diff.removed.push(...queries.map(q => ({ name, range: q.range }))); + } + + return diff; + } + async save(path: string) { await fs.writeFile(path, this.toString(), 'utf8'); } From 60196dc40fcc115a65aa98f4bc4038d23262792d Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Tue, 13 Dec 2022 22:10:51 +0100 Subject: [PATCH 03/14] cli: flip around lockfile diff Signed-off-by: Patrik Oldsberg --- .../cli/src/lib/versioning/Lockfile.test.ts | 64 +++++++++---------- packages/cli/src/lib/versioning/Lockfile.ts | 63 +++++++++++------- 2 files changed, 71 insertions(+), 56 deletions(-) diff --git a/packages/cli/src/lib/versioning/Lockfile.test.ts b/packages/cli/src/lib/versioning/Lockfile.test.ts index 8209ef1f23..b65d2c0fbe 100644 --- a/packages/cli/src/lib/versioning/Lockfile.test.ts +++ b/packages/cli/src/lib/versioning/Lockfile.test.ts @@ -416,30 +416,30 @@ d@^1: it('should diff two legacy lockfiles', async () => { expect(lockfileLegacyA.diff(lockfileLegacyB)).toEqual({ added: [ - { name: 'b', range: '4' }, - { name: 'd', range: '^1' }, + { name: 'b', range: '3' }, + { name: 'c', range: '^1' }, ], changed: [ { name: 'a', range: '^1' }, { name: 'b', range: '2.0.x' }, ], removed: [ - { name: 'b', range: '3' }, - { name: 'c', range: '^1' }, + { name: 'b', range: '4' }, + { name: 'd', range: '^1' }, ], }); expect(lockfileLegacyB.diff(lockfileLegacyA)).toEqual({ added: [ - { name: 'b', range: '3' }, - { name: 'c', range: '^1' }, + { name: 'b', range: '4' }, + { name: 'd', range: '^1' }, ], changed: [ { name: 'a', range: '^1' }, { name: 'b', range: '2.0.x' }, ], removed: [ - { name: 'b', range: '4' }, - { name: 'd', range: '^1' }, + { name: 'b', range: '3' }, + { name: 'c', range: '^1' }, ], }); }); @@ -447,30 +447,30 @@ d@^1: it('should diff two modern lockfiles', async () => { expect(lockfileModernA.diff(lockfileModernB)).toEqual({ added: [ - { name: 'b', range: '4' }, - { name: 'd', range: '^1' }, + { name: 'b', range: '3' }, + { name: 'c', range: '^1' }, ], changed: [ { name: 'a', range: '^1' }, { name: 'b', range: '2.0.x' }, ], removed: [ - { name: 'b', range: '3' }, - { name: 'c', range: '^1' }, + { name: 'b', range: '4' }, + { name: 'd', range: '^1' }, ], }); expect(lockfileModernB.diff(lockfileModernA)).toEqual({ added: [ - { name: 'b', range: '3' }, - { name: 'c', range: '^1' }, + { name: 'b', range: '4' }, + { name: 'd', range: '^1' }, ], changed: [ { name: 'a', range: '^1' }, { name: 'b', range: '2.0.x' }, ], removed: [ - { name: 'b', range: '4' }, - { name: 'd', range: '^1' }, + { name: 'b', range: '3' }, + { name: 'c', range: '^1' }, ], }); }); @@ -478,30 +478,30 @@ d@^1: it('should diff legacy and modern lockfiles', async () => { expect(lockfileLegacyA.diff(lockfileModernB)).toEqual({ added: [ - { name: 'b', range: '4' }, - { name: 'd', range: '^1' }, + { name: 'b', range: '3' }, + { name: 'c', range: '^1' }, ], changed: [ { name: 'a', range: '^1' }, { name: 'b', range: '2.0.x' }, ], removed: [ - { name: 'b', range: '3' }, - { name: 'c', range: '^1' }, + { name: 'b', range: '4' }, + { name: 'd', range: '^1' }, ], }); expect(lockfileLegacyB.diff(lockfileModernA)).toEqual({ added: [ - { name: 'b', range: '3' }, - { name: 'c', range: '^1' }, + { name: 'b', range: '4' }, + { name: 'd', range: '^1' }, ], changed: [ { name: 'a', range: '^1' }, { name: 'b', range: '2.0.x' }, ], removed: [ - { name: 'b', range: '4' }, - { name: 'd', range: '^1' }, + { name: 'b', range: '3' }, + { name: 'c', range: '^1' }, ], }); }); @@ -509,30 +509,30 @@ d@^1: it('should diff modern and legacy lockfiles', async () => { expect(lockfileModernA.diff(lockfileLegacyB)).toEqual({ added: [ - { name: 'b', range: '4' }, - { name: 'd', range: '^1' }, + { name: 'b', range: '3' }, + { name: 'c', range: '^1' }, ], changed: [ { name: 'a', range: '^1' }, { name: 'b', range: '2.0.x' }, ], removed: [ - { name: 'b', range: '3' }, - { name: 'c', range: '^1' }, + { name: 'b', range: '4' }, + { name: 'd', range: '^1' }, ], }); expect(lockfileModernB.diff(lockfileLegacyA)).toEqual({ added: [ - { name: 'b', range: '3' }, - { name: 'c', range: '^1' }, + { name: 'b', range: '4' }, + { name: 'd', range: '^1' }, ], changed: [ { name: 'a', range: '^1' }, { name: 'b', range: '2.0.x' }, ], removed: [ - { name: 'b', range: '4' }, - { name: 'd', range: '^1' }, + { name: 'b', range: '3' }, + { name: 'c', range: '^1' }, ], }); }); diff --git a/packages/cli/src/lib/versioning/Lockfile.ts b/packages/cli/src/lib/versioning/Lockfile.ts index d156c9498d..8f4bd6af19 100644 --- a/packages/cli/src/lib/versioning/Lockfile.ts +++ b/packages/cli/src/lib/versioning/Lockfile.ts @@ -38,6 +38,17 @@ type LockfileQueryEntry = { dataKey: string; }; +type LockfileDiffEntry = { + name: string; + range: string; +}; + +type LockfileDiff = { + added: LockfileDiffEntry[]; + changed: LockfileDiffEntry[]; + removed: LockfileDiffEntry[]; +}; + /** Entries that have an invalid version range, for example an npm tag */ type AnalyzeResultInvalidRange = { name: string; @@ -328,56 +339,60 @@ export class Lockfile { } } - diff(newLockfile: Lockfile) { + /** + * Diff with another lockfile, returning entries that have been + * added, changed, and removed compared to the other lockfile. + */ + diff(otherLockfile: Lockfile): LockfileDiff { const diff = { added: new Array<{ name: string; range: string }>(), - removed: new Array<{ name: string; range: string }>(), changed: new Array<{ name: string; range: string }>(), + removed: new Array<{ name: string; range: string }>(), }; - // Keeps track of packages that only exist in the old lockfile + // Keeps track of packages that only exist in this lockfile const remainingOldNames = new Set(this.packages.keys()); - for (const [name, newQueries] of newLockfile.packages) { + for (const [name, otherQueries] of otherLockfile.packages) { remainingOldNames.delete(name); - const oldQueries = this.packages.get(name); - // If the packages doesn't exist in the old lockfile, add all entries - if (!oldQueries) { - diff.added.push(...newQueries.map(q => ({ name, range: q.range }))); + const thisQueries = this.packages.get(name); + // If the packages doesn't exist in this lockfile, add all entries + if (!thisQueries) { + diff.removed.push(...otherQueries.map(q => ({ name, range: q.range }))); continue; } - const remainingOldRanges = new Set(oldQueries.map(q => q.range)); + const remainingOldRanges = new Set(thisQueries.map(q => q.range)); - for (const newQuery of newQueries) { - remainingOldRanges.delete(newQuery.range); + for (const otherQuery of otherQueries) { + remainingOldRanges.delete(otherQuery.range); - const oldQuery = oldQueries.find(q => q.range === newQuery.range); - if (!oldQuery) { - diff.added.push({ name, range: newQuery.range }); + const thisQuery = thisQueries.find(q => q.range === otherQuery.range); + if (!thisQuery) { + diff.removed.push({ name, range: otherQuery.range }); continue; } - const newPkg = newLockfile.data[newQuery.dataKey]; - const oldPkg = this.data[oldQuery.dataKey]; - if (newPkg && oldPkg) { - const oldCheck = oldPkg.integrity || oldPkg.checksum; - const newCheck = newPkg.integrity || newPkg.checksum; - if (!oldCheck || !newCheck || oldCheck !== newCheck) { - diff.changed.push({ name, range: newQuery.range }); + const otherPkg = otherLockfile.data[otherQuery.dataKey]; + const thisPkg = this.data[thisQuery.dataKey]; + if (otherPkg && thisPkg) { + const thisCheck = thisPkg.integrity || thisPkg.checksum; + const otherCheck = otherPkg.integrity || otherPkg.checksum; + if (!thisCheck || !otherCheck || thisCheck !== otherCheck) { + diff.changed.push({ name, range: otherQuery.range }); } } } - for (const oldRange of remainingOldRanges) { - diff.removed.push({ name, range: oldRange }); + for (const thisRange of remainingOldRanges) { + diff.added.push({ name, range: thisRange }); } } for (const name of remainingOldNames) { const queries = this.packages.get(name) ?? []; - diff.removed.push(...queries.map(q => ({ name, range: q.range }))); + diff.added.push(...queries.map(q => ({ name, range: q.range }))); } return diff; From 3c71bf7789e198370028c575aa08ae1e03661ad3 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Tue, 13 Dec 2022 22:26:32 +0100 Subject: [PATCH 04/14] cli: fix lockfile diff for entries without checksum Signed-off-by: Patrik Oldsberg --- .../cli/src/lib/versioning/Lockfile.test.ts | 49 +++++++++++++++++++ packages/cli/src/lib/versioning/Lockfile.ts | 2 +- 2 files changed, 50 insertions(+), 1 deletion(-) diff --git a/packages/cli/src/lib/versioning/Lockfile.test.ts b/packages/cli/src/lib/versioning/Lockfile.test.ts index b65d2c0fbe..7b0da9ce8c 100644 --- a/packages/cli/src/lib/versioning/Lockfile.test.ts +++ b/packages/cli/src/lib/versioning/Lockfile.test.ts @@ -536,5 +536,54 @@ d@^1: ], }); }); + + it('should handle workspace ranges', async () => { + const lockfile = `${HEADER} +"@backstage/app-defaults@workspace:^, @backstage/app-defaults@workspace:packages/app-defaults": + version: 0.0.0-use.local + resolution: "@backstage/app-defaults@workspace:packages/app-defaults" + dependencies: + "@backstage/cli": "workspace:^" + "@backstage/core-app-api": "workspace:^" + "@backstage/core-components": "workspace:^" + "@backstage/core-plugin-api": "workspace:^" + "@backstage/plugin-permission-react": "workspace:^" + "@backstage/test-utils": "workspace:^" + "@backstage/theme": "workspace:^" + "@material-ui/core": ^4.12.2 + "@material-ui/icons": ^4.9.1 + "@testing-library/jest-dom": ^5.10.1 + "@testing-library/react": ^12.1.3 + "@types/node": ^16.11.26 + "@types/react": ^16.13.1 || ^17.0.0 + peerDependencies: + react: ^16.13.1 || ^17.0.0 + react-dom: ^16.13.1 || ^17.0.0 + react-router-dom: 6.0.0-beta.0 || ^6.3.0 + languageName: unknown + linkType: soft + +"@backstage/backend-app-api@workspace:^, @backstage/backend-app-api@workspace:packages/backend-app-api": + version: 0.0.0-use.local + resolution: "@backstage/backend-app-api@workspace:packages/backend-app-api" + dependencies: + "@backstage/backend-common": "workspace:^" + "@backstage/backend-plugin-api": "workspace:^" + "@backstage/backend-tasks": "workspace:^" + "@backstage/cli": "workspace:^" + "@backstage/errors": "workspace:^" + "@backstage/plugin-permission-node": "workspace:^" + express: ^4.17.1 + express-promise-router: ^4.1.0 + winston: ^3.2.1 + languageName: unknown + linkType: soft +`; + expect(Lockfile.parse(lockfile).diff(Lockfile.parse(lockfile))).toEqual({ + added: [], + changed: [], + removed: [], + }); + }); }); }); diff --git a/packages/cli/src/lib/versioning/Lockfile.ts b/packages/cli/src/lib/versioning/Lockfile.ts index 8f4bd6af19..622aac36ae 100644 --- a/packages/cli/src/lib/versioning/Lockfile.ts +++ b/packages/cli/src/lib/versioning/Lockfile.ts @@ -379,7 +379,7 @@ export class Lockfile { if (otherPkg && thisPkg) { const thisCheck = thisPkg.integrity || thisPkg.checksum; const otherCheck = otherPkg.integrity || otherPkg.checksum; - if (!thisCheck || !otherCheck || thisCheck !== otherCheck) { + if (thisCheck !== otherCheck) { diff.changed.push({ name, range: otherQuery.range }); } } From 18543ed94b1caa5194839c63e2fe52f72068877e Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Tue, 13 Dec 2022 23:21:36 +0100 Subject: [PATCH 05/14] cli: fix lockfile test formats Signed-off-by: Patrik Oldsberg --- .../cli/src/lib/versioning/Lockfile.test.ts | 102 +++++++++--------- 1 file changed, 51 insertions(+), 51 deletions(-) diff --git a/packages/cli/src/lib/versioning/Lockfile.test.ts b/packages/cli/src/lib/versioning/Lockfile.test.ts index 7b0da9ce8c..30c6f6ffee 100644 --- a/packages/cli/src/lib/versioning/Lockfile.test.ts +++ b/packages/cli/src/lib/versioning/Lockfile.test.ts @@ -19,12 +19,20 @@ import mockFs from 'mock-fs'; import { ExtendedPackage } from '../monorepo'; import { Lockfile } from './Lockfile'; -const HEADER = `# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY. +const LEGACY_HEADER = `# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY. # yarn lockfile v1 `; -const mockA = `${HEADER} +const MODERN_HEADER = `# This file is generated by running "yarn install" inside your project. +# Manual changes might be lost - proceed with caution! + +__metadata: + version: 6 + cacheKey: 8 +`; + +const mockA = `${LEGACY_HEADER} a@^1: version "1.0.1" resolved "https://my-registry/a-1.0.01.tgz#abc123" @@ -39,7 +47,7 @@ b@^2: version "2.0.0" `; -const mockADedup = `${HEADER} +const mockADedup = `${LEGACY_HEADER} a@^1: version "1.0.1" resolved "https://my-registry/a-1.0.01.tgz#abc123" @@ -51,7 +59,7 @@ b@2.0.x, b@^2: version "2.0.1" `; -const mockB = `${HEADER} +const mockB = `${LEGACY_HEADER} "@s/a@*", "@s/a@1 || 2", "@s/a@^1": version "1.0.1" @@ -59,7 +67,7 @@ const mockB = `${HEADER} version "2.0.0" `; -const mockBDedup = `${HEADER} +const mockBDedup = `${LEGACY_HEADER} "@s/a@*", "@s/a@1 || 2", "@s/a@^2.0.x": version "2.0.0" @@ -157,15 +165,7 @@ describe('Lockfile', () => { }); }); -const newHeader = `# This file is generated by running "yarn install" inside your project. -# Manual changes might be lost - proceed with caution! - -__metadata: - version: 6 - cacheKey: 8 -`; - -const mockANew = `${newHeader} +const mockANew = `${MODERN_HEADER} a@^1: version: 1.0.1 dependencies: @@ -180,7 +180,7 @@ b@^2: version: 2.0.0 `; -const mockANewDedup = `${newHeader} +const mockANewDedup = `${MODERN_HEADER} a@^1: version: 1.0.1 dependencies: @@ -321,7 +321,7 @@ describe('New Lockfile', () => { }); describe('diff', () => { - const lockfileLegacyA = Lockfile.parse(`${HEADER} + const lockfileLegacyA = Lockfile.parse(`${LEGACY_HEADER} a@^1: version "1.0.1" resolved "https://my-registry/a-1.0.01.tgz#abc123" @@ -346,7 +346,7 @@ c@^1: integrity x `); - const lockfileLegacyB = Lockfile.parse(`${HEADER} + const lockfileLegacyB = Lockfile.parse(`${LEGACY_HEADER} a@^1: version "1.0.1" resolved "https://my-registry/a-1.0.01.tgz#abc123" @@ -367,50 +367,50 @@ d@^1: integrity x `); - const lockfileModernA = Lockfile.parse(`${HEADER} -"a@^1": - version "1.0.1" - resolved "https://my-registry/a-1.0.01.tgz#abc123" - checksum sha512-xyz + const lockfileModernA = Lockfile.parse(`${MODERN_HEADER} +"a@npm:^1": + version: "1.0.1" + resolved: "https://my-registry/a-1.0.01.tgz#abc123" + checksum: sha512-xyz dependencies: - b "^2" + b: "^2" -"b@3": - version "3.0.1" - checksum sha512-abc1 +"b@npm:3": + version: "3.0.1" + checksum: sha512-abc1 -"b@2.0.x": - version "2.0.1" - checksum sha512-abc2 +"b@npm:2.0.x": + version: "2.0.1" + checksum: sha512-abc2 -"b@^2": - version "2.0.0" - checksum sha512-abc3 +"b@npm:^2": + version: "2.0.0" + checksum: sha512-abc3 -"c@^1": - version "1.0.1" - checksum x +"c@npm:^1": + version: "1.0.1" + checksum: x `); - const lockfileModernB = Lockfile.parse(`${HEADER} -"a@^1": - version "1.0.1" - resolution "a@npm:1.0.1" - checksum sha512-xyz-other + const lockfileModernB = Lockfile.parse(`${MODERN_HEADER} +"a@npm:^1": + version: "1.0.1" + resolution: "a@npm:1.0.1" + checksum: sha512-xyz-other dependencies: - b "^2" + b: "^2" -"b@2.0.x, b@^2": - version "2.0.0" - checksum sha512-abc3 +"b@npm:2.0.x, b@npm:^2": + version: "2.0.0" + checksum: sha512-abc3 -"b@4": - version "4.0.0" - checksum sha512-abc +"b@npm:4": + version: "4.0.0" + checksum: sha512-abc -"d@^1": - version "1.0.1" - checksum x +"d@npm:^1": + version: "1.0.1" + checksum: x `); it('should diff two legacy lockfiles', async () => { @@ -538,7 +538,7 @@ d@^1: }); it('should handle workspace ranges', async () => { - const lockfile = `${HEADER} + const lockfile = `${MODERN_HEADER} "@backstage/app-defaults@workspace:^, @backstage/app-defaults@workspace:packages/app-defaults": version: 0.0.0-use.local resolution: "@backstage/app-defaults@workspace:packages/app-defaults" From 99898258cd30e5dfa0ab2311cfc247cb35ce5756 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Tue, 13 Dec 2022 23:21:53 +0100 Subject: [PATCH 06/14] cli: add simple dependency graph utility for lockfiles Signed-off-by: Patrik Oldsberg --- .../cli/src/lib/versioning/Lockfile.test.ts | 197 ++++++++++++++++++ packages/cli/src/lib/versioning/Lockfile.ts | 20 ++ 2 files changed, 217 insertions(+) diff --git a/packages/cli/src/lib/versioning/Lockfile.test.ts b/packages/cli/src/lib/versioning/Lockfile.test.ts index 30c6f6ffee..a60b71be85 100644 --- a/packages/cli/src/lib/versioning/Lockfile.test.ts +++ b/packages/cli/src/lib/versioning/Lockfile.test.ts @@ -586,4 +586,201 @@ d@^1: }); }); }); + + describe('createSimplifiedDependencyGraph', () => { + it('for modern lockfile', () => { + expect( + Lockfile.parse( + `${MODERN_HEADER} +"@backstage/app-defaults@workspace:^, @backstage/app-defaults@workspace:packages/app-defaults": + version: 0.0.0-use.local + resolution: "@backstage/app-defaults@workspace:packages/app-defaults" + dependencies: + "@backstage/cli": "workspace:^" + "@backstage/core-app-api": "workspace:^" + "@backstage/core-components": "workspace:^" + "@backstage/core-plugin-api": "workspace:^" + "@backstage/plugin-permission-react": "workspace:^" + "@backstage/test-utils": "workspace:^" + "@backstage/theme": "workspace:^" + "@material-ui/core": ^4.12.2 + "@material-ui/icons": ^4.9.1 + "@testing-library/jest-dom": ^5.10.1 + "@testing-library/react": ^12.1.3 + "@types/node": ^16.11.26 + "@types/react": ^16.13.1 || ^17.0.0 + peerDependencies: + react: ^16.13.1 || ^17.0.0 + react-dom: ^16.13.1 || ^17.0.0 + react-router-dom: 6.0.0-beta.0 || ^6.3.0 + languageName: unknown + linkType: soft + +"@backstage/backend-app-api@workspace:^, @backstage/backend-app-api@workspace:packages/backend-app-api": + version: 0.0.0-use.local + resolution: "@backstage/backend-app-api@workspace:packages/backend-app-api" + dependencies: + "@backstage/backend-common": "workspace:^" + "@backstage/backend-plugin-api": "workspace:^" + "@backstage/backend-tasks": "workspace:^" + "@backstage/cli": "workspace:^" + "@backstage/errors": "workspace:^" + "@backstage/plugin-permission-node": "workspace:^" + express: ^4.17.1 + express-promise-router: ^4.1.0 + winston: ^3.2.1 + languageName: unknown + linkType: soft +`, + ).createSimplifiedDependencyGraph(), + ).toEqual( + new Map([ + [ + '@backstage/app-defaults', + new Set([ + '@backstage/cli', + '@backstage/core-app-api', + '@backstage/core-components', + '@backstage/core-plugin-api', + '@backstage/plugin-permission-react', + '@backstage/test-utils', + '@backstage/theme', + '@material-ui/core', + '@material-ui/icons', + '@testing-library/jest-dom', + '@testing-library/react', + '@types/node', + '@types/react', + 'react', + 'react-dom', + 'react-router-dom', + ]), + ], + [ + '@backstage/backend-app-api', + new Set([ + '@backstage/backend-common', + '@backstage/backend-plugin-api', + '@backstage/backend-tasks', + '@backstage/cli', + '@backstage/errors', + '@backstage/plugin-permission-node', + 'express', + 'express-promise-router', + 'winston', + ]), + ], + ]), + ); + }); + + it('for simple lockfile without dependencies', () => { + expect( + Lockfile.parse( + `${MODERN_HEADER} +"a@npm:^1": + version: "1.0.1" + +"b@npm:3": + version: "3.0.1" + +"b@npm:2.0.x": + version: "2.0.1" + checksum: sha512-abc2 +`, + ).createSimplifiedDependencyGraph(), + ).toEqual( + new Map([ + ['a', new Set()], + ['b', new Set()], + ]), + ); + }); + + it('for lockfile with dependencies', () => { + expect( + Lockfile.parse( + `${MODERN_HEADER} +"a@npm:^1": + version: "1.0.1" + dependencies: + b: "^2" + +"b@npm:3": + version: "3.0.1" + checksum: sha512-abc1 + +"b@npm:2.0.x": + version: "2.0.1" + checksum: sha512-abc2 + dependencies: + c: "^1" + +"b@npm:^2": + version: "2.0.0" + checksum: sha512-abc3 + peerDependencies: + d: "^1" + +"c@npm:^1": + version: "1.0.1" + +"d@npm:^1": + version: "1.0.2" +`, + ).createSimplifiedDependencyGraph(), + ).toEqual( + new Map([ + ['a', new Set(['b'])], + ['b', new Set(['c', 'd'])], + ['c', new Set()], + ['d', new Set()], + ]), + ); + }); + + it('for legacy lockfile', () => { + expect( + Lockfile.parse( + `${LEGACY_HEADER} +a@^1: + version "1.0.1" + dependencies: + b "^2" + +b@3: + version "3.0.1" + integrity sha512-abc1 + +b@2.0.x: + version "2.0.1" + integrity sha512-abc2 + dependencies: + c "^1" + +b@^2: + version "2.0.0" + integrity sha512-abc3 + dependencies: + d "^1" + +c@^1: + version "1.0.1" + integrity x + +d@^1: + version "1.0.1" + integrity x +`, + ).createSimplifiedDependencyGraph(), + ).toEqual( + new Map([ + ['a', new Set(['b'])], + ['b', new Set(['c', 'd'])], + ['c', new Set()], + ['d', new Set()], + ]), + ); + }); + }); }); diff --git a/packages/cli/src/lib/versioning/Lockfile.ts b/packages/cli/src/lib/versioning/Lockfile.ts index 622aac36ae..4eadaf55f9 100644 --- a/packages/cli/src/lib/versioning/Lockfile.ts +++ b/packages/cli/src/lib/versioning/Lockfile.ts @@ -29,6 +29,7 @@ type LockfileData = { integrity?: string /* old */; checksum?: string /* new */; dependencies?: { [name: string]: string }; + peerDependencies?: { [name: string]: string }; }; }; @@ -339,6 +340,25 @@ export class Lockfile { } } + createSimplifiedDependencyGraph(): Map> { + const graph = new Map>(); + + for (const [name, entries] of this.packages) { + const dependencies = new Set( + entries.flatMap(e => { + const data = this.data[e.dataKey]; + return [ + ...Object.keys(data?.dependencies ?? {}), + ...Object.keys(data?.peerDependencies ?? {}), + ]; + }), + ); + graph.set(name, dependencies); + } + + return graph; + } + /** * Diff with another lockfile, returning entries that have been * added, changed, and removed compared to the other lockfile. From a11a8082ce2dd19539ad5f0f89997bc197037699 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Tue, 13 Dec 2022 23:22:44 +0100 Subject: [PATCH 07/14] cli: add git utility for reading a file at a ref Signed-off-by: Patrik Oldsberg --- packages/cli/src/lib/git.ts | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/packages/cli/src/lib/git.ts b/packages/cli/src/lib/git.ts index 074260b38f..c0b50f5140 100644 --- a/packages/cli/src/lib/git.ts +++ b/packages/cli/src/lib/git.ts @@ -64,3 +64,23 @@ export async function listChangedFiles(ref: string) { return Array.from(new Set([...tracked, ...untracked])); } + +/** + * Returns the contents of a file at a specific ref. + */ +export async function readFileAtRef(path: string, ref: string) { + let showRef = ref; + try { + const [base] = await runGit('merge-base', 'HEAD', ref); + showRef = base; + } catch { + // silently fall back to using the ref directly if merge base is not available + } + + const { stdout } = await execFile('git', ['show', `${showRef}:${path}`], { + shell: true, + cwd: paths.targetRoot, + maxBuffer: 1024 * 1024 * 50, + }); + return stdout; +} From 79bc82828ff1e1ff0ce9c855d8a1a48caeb46555 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Tue, 13 Dec 2022 23:23:11 +0100 Subject: [PATCH 08/14] cli: add ability to analyze lockfile changes when listing changed packages Signed-off-by: Patrik Oldsberg --- packages/cli/src/lib/monorepo/PackageGraph.ts | 70 ++++++++++++++++++- 1 file changed, 68 insertions(+), 2 deletions(-) diff --git a/packages/cli/src/lib/monorepo/PackageGraph.ts b/packages/cli/src/lib/monorepo/PackageGraph.ts index c65f5c003c..8aa7fc9416 100644 --- a/packages/cli/src/lib/monorepo/PackageGraph.ts +++ b/packages/cli/src/lib/monorepo/PackageGraph.ts @@ -18,7 +18,8 @@ import path from 'path'; import { getPackages, Package } from '@manypkg/get-packages'; import { paths } from '../paths'; import { PackageRole } from '../role'; -import { listChangedFiles } from '../git'; +import { listChangedFiles, readFileAtRef } from '../git'; +import { Lockfile } from '../versioning'; type PackageJSON = Package['packageJson']; @@ -191,7 +192,10 @@ export class PackageGraph extends Map { return targets; } - async listChangedPackages(options: { ref: string }) { + async listChangedPackages(options: { + ref: string; + analyzeLockfile?: boolean; + }) { const changedFiles = await listChangedFiles(options.ref); const dirMap = new Map( @@ -234,6 +238,68 @@ export class PackageGraph extends Map { } } + if (changedFiles.includes('yarn.lock') && options.analyzeLockfile) { + // Load the lockfile in the working tree and the one at the ref and diff them + const thisLockfile = await Lockfile.load( + paths.resolveTargetRoot('yarn.lock'), + ); + const otherLockfile = Lockfile.parse( + await readFileAtRef('yarn.lock', options.ref), + ); + const diff = thisLockfile.diff(otherLockfile); + + // Create a simplified dependency graph only keeps track of package names + const graph = thisLockfile.createSimplifiedDependencyGraph(); + + // Merge the dependency graph from the other lockfile into this one in + // order to be able to detect removals accurately. + { + const otherGraph = thisLockfile.createSimplifiedDependencyGraph(); + for (const [name, dependencies] of otherGraph) { + const node = graph.get(name); + if (node) { + dependencies.forEach(d => node.add(d)); + } else { + graph.set(name, dependencies); + } + } + } + + // The check is simplified by only considering the package names rather + // than the exact version range queries that were changed. + // TODO(Rugvip): Use a more exact check + const changedPackages = new Set( + [...diff.added, ...diff.changed, ...diff.removed].map(e => e.name), + ); + + // Starting with our set of changed packages from the diff, we loop through + // the full graph and add any package that has a dependency on a changed package. + // We keep looping until all transitive dependencies have been detected. + let changed = false; + do { + changed = false; + for (const [name, dependencies] of graph) { + if (changedPackages.has(name)) { + continue; + } + for (const dep of dependencies) { + if (changedPackages.has(dep)) { + changed = true; + changedPackages.add(name); + break; + } + } + } + } while (changed); + + // Add all local packages that had a transitive dependency change to the result set + for (const node of this.values()) { + if (changedPackages.has(node.name) && !result.includes(node)) { + result.push(node); + } + } + } + return result; } } From 7c8a974515c75e5e18dcd58523f35f6ec76385fb Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Tue, 13 Dec 2022 23:32:30 +0100 Subject: [PATCH 09/14] cli: make --since flag for repo build, lint and test commands diff yarn lock Signed-off-by: Patrik Oldsberg --- .changeset/tasty-impalas-cross.md | 5 +++++ packages/cli/src/commands/repo/build.ts | 1 + packages/cli/src/commands/repo/lint.ts | 5 ++++- packages/cli/src/commands/repo/test.ts | 1 + 4 files changed, 11 insertions(+), 1 deletion(-) create mode 100644 .changeset/tasty-impalas-cross.md diff --git a/.changeset/tasty-impalas-cross.md b/.changeset/tasty-impalas-cross.md new file mode 100644 index 0000000000..f7d7712b29 --- /dev/null +++ b/.changeset/tasty-impalas-cross.md @@ -0,0 +1,5 @@ +--- +'@backstage/cli': patch +--- + +The `repo test`, `repo lint`, and `repo build` commands will now analyze `yarn.lock` for dependency changes when searching for changed packages. This allows you to use the `--since ` flag even if you have `yarn.lock` changes. diff --git a/packages/cli/src/commands/repo/build.ts b/packages/cli/src/commands/repo/build.ts index 21b1580cb9..a6bf87da82 100644 --- a/packages/cli/src/commands/repo/build.ts +++ b/packages/cli/src/commands/repo/build.ts @@ -84,6 +84,7 @@ export async function command(opts: OptionValues, cmd: Command): Promise { const graph = PackageGraph.fromPackages(packages); const changedPackages = await graph.listChangedPackages({ ref: opts.since, + analyzeLockfile: true, }); const withDevDependents = graph.collectPackageNames( changedPackages.map(pkg => pkg.name), diff --git a/packages/cli/src/commands/repo/lint.ts b/packages/cli/src/commands/repo/lint.ts index 61d85b3773..8e8ec20ff8 100644 --- a/packages/cli/src/commands/repo/lint.ts +++ b/packages/cli/src/commands/repo/lint.ts @@ -34,7 +34,10 @@ export async function command(opts: OptionValues): Promise { if (opts.since) { const graph = PackageGraph.fromPackages(packages); - packages = await graph.listChangedPackages({ ref: opts.since }); + packages = await graph.listChangedPackages({ + ref: opts.since, + analyzeLockfile: true, + }); } // Packages are ordered from most to least number of dependencies, as a diff --git a/packages/cli/src/commands/repo/test.ts b/packages/cli/src/commands/repo/test.ts index 578cb15e83..95bfe8f449 100644 --- a/packages/cli/src/commands/repo/test.ts +++ b/packages/cli/src/commands/repo/test.ts @@ -93,6 +93,7 @@ export async function command(opts: OptionValues, cmd: Command): Promise { const graph = PackageGraph.fromPackages(packages); const changedPackages = await graph.listChangedPackages({ ref: opts.since, + analyzeLockfile: true, }); const packageNames = Array.from( From 536d3e764af41a4600982d0a96200fd0549af26d Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Tue, 13 Dec 2022 23:45:08 +0100 Subject: [PATCH 10/14] cli: fix missing arg for lockfile.save() Signed-off-by: Patrik Oldsberg --- packages/cli/src/commands/versions/lint.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/cli/src/commands/versions/lint.ts b/packages/cli/src/commands/versions/lint.ts index e6bc119b71..8de38548a9 100644 --- a/packages/cli/src/commands/versions/lint.ts +++ b/packages/cli/src/commands/versions/lint.ts @@ -53,7 +53,8 @@ export default async (cmd: OptionValues) => { let success = true; - const lockfile = await Lockfile.load(paths.resolveTargetRoot('yarn.lock')); + const lockfilePath = paths.resolveTargetRoot('yarn.lock'); + const lockfile = await Lockfile.load(lockfilePath); const result = lockfile.analyze({ filter: includedFilter, localPackages: PackageGraph.fromPackages( @@ -69,7 +70,7 @@ export default async (cmd: OptionValues) => { if (fix) { lockfile.replaceVersions(result.newVersions); - await lockfile.save(); + await lockfile.save(lockfilePath); } else { const [newVersionsForbidden, newVersionsAllowed] = partition( result.newVersions, From 1d0304de1b09ffb010d7546882e2daa2525b3e86 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Tue, 13 Dec 2022 23:45:31 +0100 Subject: [PATCH 11/14] workflows/ci: remove yarn.lock check and always use --since Signed-off-by: Patrik Oldsberg --- .github/workflows/ci.yml | 22 ---------------------- 1 file changed, 22 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a37ae745c4..c4af6021b2 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -185,21 +185,10 @@ jobs: with: cache-prefix: ${{ runner.os }}-v${{ matrix.node-version }} - - name: check for yarn.lock changes - id: yarn-lock - run: git diff --quiet origin/master HEAD -- yarn.lock - continue-on-error: true - - name: lint changed packages - if: ${{ steps.yarn-lock.outcome == 'success' }} run: yarn backstage-cli repo lint --since origin/master - - name: lint all packages - if: ${{ steps.yarn-lock.outcome == 'failure' }} - run: yarn backstage-cli repo lint - - name: test changed packages - if: ${{ steps.yarn-lock.outcome == 'success' }} run: yarn backstage-cli repo test --maxWorkers=2 --workerIdleMemoryLimit=1300M --since origin/master env: BACKSTAGE_TEST_DISABLE_DOCKER: 1 @@ -207,17 +196,6 @@ jobs: BACKSTAGE_TEST_DATABASE_POSTGRES9_CONNECTION_STRING: postgresql://postgres:postgres@localhost:${{ job.services.postgres9.ports[5432] }} BACKSTAGE_TEST_DATABASE_MYSQL8_CONNECTION_STRING: mysql://root:root@localhost:${{ job.services.mysql8.ports[3306] }}/ignored - - name: test all packages (and upload coverage) - if: ${{ steps.yarn-lock.outcome == 'failure' }} - run: | - yarn backstage-cli repo test --maxWorkers=2 --workerIdleMemoryLimit=800M --coverage - bash <(curl -s https://codecov.io/bash) -N $(git rev-parse FETCH_HEAD) - env: - BACKSTAGE_TEST_DISABLE_DOCKER: 1 - BACKSTAGE_TEST_DATABASE_POSTGRES13_CONNECTION_STRING: postgresql://postgres:postgres@localhost:${{ job.services.postgres13.ports[5432] }} - BACKSTAGE_TEST_DATABASE_POSTGRES9_CONNECTION_STRING: postgresql://postgres:postgres@localhost:${{ job.services.postgres9.ports[5432] }} - BACKSTAGE_TEST_DATABASE_MYSQL8_CONNECTION_STRING: mysql://root:root@localhost:${{ job.services.mysql8.ports[3306] }}/ignored - - name: ensure clean working directory run: | if files=$(git ls-files --exclude-standard --others --modified) && [[ -z "$files" ]]; then From 7bd873ad15bc66644fd7fb5e590722129473e285 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Wed, 14 Dec 2022 00:05:50 +0100 Subject: [PATCH 12/14] cli: handle failure to read lockfile during analysis Signed-off-by: Patrik Oldsberg --- packages/cli/src/lib/monorepo/PackageGraph.ts | 21 +++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/packages/cli/src/lib/monorepo/PackageGraph.ts b/packages/cli/src/lib/monorepo/PackageGraph.ts index 8aa7fc9416..a1fc95e669 100644 --- a/packages/cli/src/lib/monorepo/PackageGraph.ts +++ b/packages/cli/src/lib/monorepo/PackageGraph.ts @@ -240,12 +240,21 @@ export class PackageGraph extends Map { if (changedFiles.includes('yarn.lock') && options.analyzeLockfile) { // Load the lockfile in the working tree and the one at the ref and diff them - const thisLockfile = await Lockfile.load( - paths.resolveTargetRoot('yarn.lock'), - ); - const otherLockfile = Lockfile.parse( - await readFileAtRef('yarn.lock', options.ref), - ); + let thisLockfile: Lockfile; + let otherLockfile: Lockfile; + try { + thisLockfile = await Lockfile.load( + paths.resolveTargetRoot('yarn.lock'), + ); + otherLockfile = Lockfile.parse( + await readFileAtRef('yarn.lock', options.ref), + ); + } catch (error) { + console.warn( + `Failed to read lockfiles, assuming all packages have changed, ${error}`, + ); + return Array.from(this.values()); + } const diff = thisLockfile.diff(otherLockfile); // Create a simplified dependency graph only keeps track of package names From 39d192d016d8b4a2772137ba518cbab86ad91a45 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Tue, 20 Dec 2022 16:48:47 +0100 Subject: [PATCH 13/14] cli: added PackageGraph test for lockfile analysis Signed-off-by: Patrik Oldsberg --- .../cli/src/lib/monorepo/PackageGraph.test.ts | 58 ++++++++++++++++++- 1 file changed, 57 insertions(+), 1 deletion(-) diff --git a/packages/cli/src/lib/monorepo/PackageGraph.test.ts b/packages/cli/src/lib/monorepo/PackageGraph.test.ts index feb1709bf7..d8edff9c81 100644 --- a/packages/cli/src/lib/monorepo/PackageGraph.test.ts +++ b/packages/cli/src/lib/monorepo/PackageGraph.test.ts @@ -14,19 +14,25 @@ * limitations under the License. */ +import { resolve as resolvePath } from 'path'; import { getPackages } from '@manypkg/get-packages'; import { PackageGraph } from './PackageGraph'; -import { listChangedFiles } from '../git'; +import { Lockfile } from '../versioning/Lockfile'; +import { listChangedFiles, readFileAtRef } from '../git'; jest.mock('../git'); const mockListChangedFiles = listChangedFiles as jest.MockedFunction< typeof listChangedFiles >; +const mockReadFileAtRef = readFileAtRef as jest.MockedFunction< + typeof readFileAtRef +>; jest.mock('../paths', () => ({ paths: { targetRoot: '/', + resolveTargetRoot: (...paths: string[]) => resolvePath('/', ...paths), }, })); @@ -177,4 +183,54 @@ describe('PackageGraph', () => { graph.listChangedPackages({ ref: 'origin/master' }), ).resolves.toEqual([graph.get('a'), graph.get('b')]); }); + + it('lists changed packages with lockfile analysis', async () => { + const graph = PackageGraph.fromPackages(testPackages); + + mockListChangedFiles.mockResolvedValueOnce( + ['README.md', 'packages/a/src/foo.ts', 'yarn.lock'].sort(), + ); + mockReadFileAtRef.mockResolvedValueOnce(` +a@^1: + version: "1.0.0" + +c@^1: + version: "1.0.0" + dependencies: + c-dep: ^1 + +c-dep@^2: + version: "2.0.0" + integrity: sha512-xyz +`); + jest.spyOn(Lockfile, 'load').mockResolvedValueOnce( + Lockfile.parse(` +a@^1: + version: "1.0.0" + +c@^1: + version: "1.0.0" + dependencies: + c-dep: ^1 + +c-dep@^2: + version: "2.0.0" + integrity: sha512-xyz-other +`), + ); + + await expect( + graph + .listChangedPackages({ + ref: 'origin/master', + analyzeLockfile: true, + }) + .then(pkgs => pkgs.map(pkg => pkg.name)), + ).resolves.toEqual(['a', 'c']); + + expect(mockReadFileAtRef).toHaveBeenCalledWith( + 'yarn.lock', + 'origin/master', + ); + }); }); From ea91c01cc9638538f6861fe0bbe23abfde0b2417 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Thu, 22 Dec 2022 14:54:08 +0100 Subject: [PATCH 14/14] cli: fix merge conflicts in Lockfile tests Signed-off-by: Patrik Oldsberg --- packages/cli/src/lib/versioning/Lockfile.test.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/cli/src/lib/versioning/Lockfile.test.ts b/packages/cli/src/lib/versioning/Lockfile.test.ts index a60b71be85..0275c5ee50 100644 --- a/packages/cli/src/lib/versioning/Lockfile.test.ts +++ b/packages/cli/src/lib/versioning/Lockfile.test.ts @@ -195,7 +195,7 @@ b@^2: version: 2.0.1 `; -const mockANewLocal = `${newHeader} +const mockANewLocal = `${MODERN_HEADER} a@^1: version: 1.0.1 dependencies: @@ -210,7 +210,7 @@ b@^2: version: 2.0.0 `; -const mockANewLocalDedup = `${newHeader} +const mockANewLocalDedup = `${MODERN_HEADER} a@^1: version: 1.0.1 dependencies: @@ -314,7 +314,7 @@ describe('New Lockfile', () => { await expect(fs.readFile('/yarn.lock', 'utf8')).resolves.toBe( mockANewLocal, ); - await expect(lockfile.save()).resolves.toBeUndefined(); + await expect(lockfile.save('/yarn.lock')).resolves.toBeUndefined(); await expect(fs.readFile('/yarn.lock', 'utf8')).resolves.toBe( mockANewLocalDedup, );