From da81c9cb54bb675855bb1785d5b6769399e58979 Mon Sep 17 00:00:00 2001 From: George Kalpakas Date: Mon, 4 Feb 2019 16:45:45 +0200 Subject: [PATCH] build(docs-infra): use pinned dependencies when possible in `ng-packages-installer` (#28510) Previously, `ng-packages-installer` would replace the version ranges for all dependencies that were peer dependencies of an Angular package with the version range used in the Angular package. This effectively meant that the pinned version (from `yarn.lock`) for that dependency was ignored (even if the pinned version satisfied the new version range). This commit reduces non-determinism in CI jobs using the locally built Angular packages by always using pinned versions of dependencies for Angular package peer dependencies if possible. For example, assuming the following versions for the RxJS dependency: - **aio/package.json**: `rxjs: ^6.3.0` - **aio/yarn.lock**: `rxjs@^6.3.0: 6.3.3` - **@angular/core#peerDependencies**: `rxjs: ^6.0.0` ...the following versions would be used with `ng-packages-installer`: - Before this commit: - **aio/package.json**: `rxjs: ^6.0.0` - **node_modules/rxjs/**: `6.4.0` (latest version satisfying `^6.0.0`) - After this commit: - **aio/package.json**: `rxjs: ^6.3.0` - **node_modules/rxjs/**: `6.3.3` (because it satisfies `^6.0.0`) PR Close #28510 --- aio/package.json | 1 + aio/tools/ng-packages-installer/index.js | 41 +++++++++-- aio/tools/ng-packages-installer/index.spec.js | 73 +++++++++++++++++-- aio/yarn.lock | 2 +- 4 files changed, 103 insertions(+), 14 deletions(-) diff --git a/aio/package.json b/aio/package.json index e845a3e796..62adff46a5 100644 --- a/aio/package.json +++ b/aio/package.json @@ -104,6 +104,7 @@ "@types/jasmine": "^2.5.52", "@types/jasminewd2": "^2.0.4", "@types/node": "~6.0.60", + "@yarnpkg/lockfile": "^1.1.0", "archiver": "^1.3.0", "canonical-path": "1.0.0", "chalk": "^2.1.0", diff --git a/aio/tools/ng-packages-installer/index.js b/aio/tools/ng-packages-installer/index.js index 25b67ff9d3..7a913ba4d9 100644 --- a/aio/tools/ng-packages-installer/index.js +++ b/aio/tools/ng-packages-installer/index.js @@ -2,11 +2,14 @@ const chalk = require('chalk'); const fs = require('fs-extra'); +const lockfile = require('@yarnpkg/lockfile'); const path = require('canonical-path'); +const semver = require('semver'); const shelljs = require('shelljs'); const yargs = require('yargs'); const PACKAGE_JSON = 'package.json'; +const YARN_LOCK = 'yarn.lock'; const LOCAL_MARKER_PATH = 'node_modules/_local_.json'; const PACKAGE_JSON_REGEX = /^[^/]+\/package\.json$/; @@ -64,6 +67,8 @@ class NgPackagesInstaller { installLocalDependencies() { if (this.force || !this._checkLocalMarker()) { const pathToPackageConfig = path.resolve(this.projectDir, PACKAGE_JSON); + const pathToLockfile = path.resolve(this.projectDir, YARN_LOCK); + const parsedLockfile = this._parseLockfile(pathToLockfile); const packages = this._getDistPackages(); try { @@ -97,8 +102,8 @@ class NgPackagesInstaller { const [dependencies, peers] = this._collectDependencies(packageConfig.dependencies || {}, packages); const [devDependencies, devPeers] = this._collectDependencies(packageConfig.devDependencies || {}, packages); - this._assignPeerDependencies(peers, dependencies, devDependencies); - this._assignPeerDependencies(devPeers, dependencies, devDependencies); + this._assignPeerDependencies(peers, dependencies, devDependencies, parsedLockfile); + this._assignPeerDependencies(devPeers, dependencies, devDependencies, parsedLockfile); const localPackageConfig = Object.assign(Object.create(null), packageConfig, { dependencies, devDependencies }); localPackageConfig.__angular = { local: true }; @@ -134,15 +139,23 @@ class NgPackagesInstaller { // Protected helpers - _assignPeerDependencies(peerDependencies, dependencies, devDependencies) { + _assignPeerDependencies(peerDependencies, dependencies, devDependencies, parsedLockfile) { Object.keys(peerDependencies).forEach(key => { + const peerDepRange = peerDependencies[key]; + + // Ignore peerDependencies whose range is already satisfied by current version in lockfile. + const originalRange = dependencies[key] || devDependencies[key]; + const lockfileVersion = originalRange && parsedLockfile[`${key}@${originalRange}`].version; + + if (lockfileVersion && semver.satisfies(lockfileVersion, peerDepRange)) return; + // If there is already an equivalent dependency then override it - otherwise assign/override the devDependency if (dependencies[key]) { - this._log(`Overriding dependency with peerDependency: ${key}: ${peerDependencies[key]}`); - dependencies[key] = peerDependencies[key]; + this._log(`Overriding dependency with peerDependency: ${key}: ${peerDepRange}`); + dependencies[key] = peerDepRange; } else { - this._log(`${devDependencies[key] ? 'Overriding' : 'Assigning'} devDependency with peerDependency: ${key}: ${peerDependencies[key]}`); - devDependencies[key] = peerDependencies[key]; + this._log(`${devDependencies[key] ? 'Overriding' : 'Assigning'} devDependency with peerDependency: ${key}: ${peerDepRange}`); + devDependencies[key] = peerDepRange; } }); } @@ -221,6 +234,20 @@ class NgPackagesInstaller { } } + /** + * Parse and return a `yarn.lock` file. + */ + _parseLockfile(lockfilePath) { + const lockfileContent = fs.readFileSync(lockfilePath, 'utf8'); + const parsed = lockfile.parse(lockfileContent); + + if (parsed.type !== 'success') { + throw new Error(`[${NgPackagesInstaller.name}]: Error parsing lockfile '${lockfilePath}' (result type: ${parsed.type}).`); + } + + return parsed.object; + } + _printWarning() { const relativeScriptPath = path.relative('.', __filename.replace(/\.js$/, '')); const absoluteProjectDir = path.resolve(this.projectDir); diff --git a/aio/tools/ng-packages-installer/index.spec.js b/aio/tools/ng-packages-installer/index.spec.js index 1a0bb8ad5d..681abd1d9f 100644 --- a/aio/tools/ng-packages-installer/index.spec.js +++ b/aio/tools/ng-packages-installer/index.spec.js @@ -1,6 +1,7 @@ 'use strict'; const fs = require('fs-extra'); +const lockfile = require('@yarnpkg/lockfile'); const path = require('canonical-path'); const shelljs = require('shelljs'); @@ -11,6 +12,7 @@ describe('NgPackagesInstaller', () => { const absoluteRootDir = path.resolve(rootDir); const nodeModulesDir = path.resolve(absoluteRootDir, 'node_modules'); const packageJsonPath = path.resolve(absoluteRootDir, 'package.json'); + const yarnLockPath = path.resolve(absoluteRootDir, 'yarn.lock'); const packagesDir = path.resolve(path.resolve(__dirname, '../../../dist/packages-dist')); const toolsDir = path.resolve(path.resolve(__dirname, '../../../dist/tools/@angular')); let installer; @@ -55,12 +57,23 @@ describe('NgPackagesInstaller', () => { spyOn(installer, '_installDeps'); spyOn(installer, '_setLocalMarker'); + spyOn(installer, '_parseLockfile').and.returnValue({ + 'rxjs@^6.3.0': {version: '6.3.3'}, + 'zone.js@^0.8.26': {version: '0.8.27'} + }); + // These are the packages that are "found" in the dist directory dummyNgPackages = { '@angular/core': { parentDir: packagesDir, packageJsonPath: `${packagesDir}/core/package.json`, - config: { peerDependencies: { 'some-package': '5.0.1' } } + config: { + peerDependencies: { + 'rxjs': '^6.4.0', + 'some-package': '5.0.1', + 'zone.js': '~0.8.26' + } + } }, '@angular/common': { parentDir: packagesDir, @@ -95,10 +108,12 @@ describe('NgPackagesInstaller', () => { dummyPackage = { dependencies: { '@angular/core': '4.4.1', - '@angular/common': '4.4.1' + '@angular/common': '4.4.1', + rxjs: '^6.3.0' }, devDependencies: { - '@angular/compiler-cli': '4.4.1' + '@angular/compiler-cli': '4.4.1', + 'zone.js': '^0.8.26' } }; dummyPackageJson = JSON.stringify(dummyPackage); @@ -106,14 +121,23 @@ describe('NgPackagesInstaller', () => { // This is the package.json that is temporarily written to the "test" folder // Note that the Angular (dev)dependencies have been modified to use a "file:" path - // And that the peerDependencies from `dummyNgPackages` have been added as (dev)dependencies. + // And that the peerDependencies from `dummyNgPackages` have been updated or added as + // (dev)dependencies (unless the current version in lockfile satisfies semver). + // + // For example, `zone.js@0.8.27` (from lockfile) satisfies `zone.js@~0.8.26` (from + // `@angular/core`), thus `zone.js: ^0.8.26` (from original `package.json`) is retained. + // In contrast, `rxjs@6.3.3` (from lockfile) does not satisfy `rxjs@^6.4.0 (from + // `@angular/core`), thus `rxjs: ^6.3.0` (from original `package.json`) is replaced with + // `rxjs: ^6.4.0` (from `@angular/core`). expectedModifiedPackage = { dependencies: { '@angular/core': `file:${packagesDir}/core`, - '@angular/common': `file:${packagesDir}/common` + '@angular/common': `file:${packagesDir}/common`, + 'rxjs': '^6.4.0' }, devDependencies: { '@angular/compiler-cli': `file:${toolsDir}/compiler-cli`, + 'zone.js': '^0.8.26', 'some-package': '5.0.1', typescript: '^2.4.2' }, @@ -150,8 +174,9 @@ describe('NgPackagesInstaller', () => { installer.installLocalDependencies(); }); - it('should get the dist packages', () => { + it('should parse the lockfile and get the dist packages', () => { expect(installer._checkLocalMarker).toHaveBeenCalled(); + expect(installer._parseLockfile).toHaveBeenCalledWith(yarnLockPath); expect(installer._getDistPackages).toHaveBeenCalled(); }); @@ -274,6 +299,42 @@ describe('NgPackagesInstaller', () => { }); }); + describe('_parseLockfile()', () => { + let originalLockfileParseDescriptor; + + beforeEach(() => { + // Workaround for `lockfile.parse()` being non-writable. + let parse = lockfile.parse; + originalLockfileParseDescriptor = Object.getOwnPropertyDescriptor(lockfile, 'parse'); + Object.defineProperty(lockfile, 'parse', { + get() { return parse; }, + set(newParse) { parse = newParse; }, + }); + + fs.readFileSync.and.returnValue('mock content'); + spyOn(lockfile, 'parse').and.returnValue({type: 'success', object: {foo: {version: 'bar'}}}); + }); + + afterEach(() => Object.defineProperty(lockfile, 'parse', originalLockfileParseDescriptor)); + + it('should parse the specified lockfile', () => { + installer._parseLockfile('/foo/bar/yarn.lock'); + expect(fs.readFileSync).toHaveBeenCalledWith('/foo/bar/yarn.lock', 'utf8'); + expect(lockfile.parse).toHaveBeenCalledWith('mock content'); + }); + + it('should throw if parsing the lockfile fails', () => { + lockfile.parse.and.returnValue({type: 'not success'}); + expect(() => installer._parseLockfile('/foo/bar/yarn.lock')).toThrowError( + '[NgPackagesInstaller]: Error parsing lockfile \'/foo/bar/yarn.lock\' (result type: not success).'); + }); + + it('should return the parsed lockfile content as an object', () => { + const parsed = installer._parseLockfile('/foo/bar/yarn.lock'); + expect(parsed).toEqual({foo: {version: 'bar'}}); + }); + }); + describe('_printWarning()', () => { it('should mention the message passed in the warning', () => { installer._printWarning(); diff --git a/aio/yarn.lock b/aio/yarn.lock index 00857867ca..81fdc4317f 100644 --- a/aio/yarn.lock +++ b/aio/yarn.lock @@ -537,7 +537,7 @@ version "4.2.1" resolved "https://registry.yarnpkg.com/@xtuc/long/-/long-4.2.1.tgz#5c85d662f76fa1d34575766c5dcd6615abcd30d8" -"@yarnpkg/lockfile@1.1.0": +"@yarnpkg/lockfile@1.1.0", "@yarnpkg/lockfile@^1.1.0": version "1.1.0" resolved "https://registry.yarnpkg.com/@yarnpkg/lockfile/-/lockfile-1.1.0.tgz#e77a97fbd345b76d83245edcd17d393b1b41fb31" integrity sha512-GpSwvyXOcOOlV70vbnzjj4fW5xW/FdUF6nQEt1ENy7m4ZCczi1+/buVUPAqmGfqznsORNFzUMjctTIp8a9tuCQ==