From 4c7d8332335785ff6a37d56199565ae55d77b84b Mon Sep 17 00:00:00 2001 From: George Kalpakas Date: Sat, 29 Feb 2020 19:14:34 +0200 Subject: [PATCH] refactor(docs-infra): simplify `NgPackagesInstaller` and speed up local package detection (#35780) Previously, `NgPackagesInstaller` would only look for Angular local packages and do so by listing all (deeply nested) files in `dist/packages-dist/` and looking for `package.json` files nested two levels deep (i.e. `dist/packages-dist/*/package.json`). Thus, it would unnecessarily check a large number of files. This commit changes the package detection logic to instead look for a `package.json` file directly inside each subdirectory of `dist/packages-dist/`, which speeds up the operation. It also refactors the code to make it easier to look for packages in other directories (besides `dist/packages-dist/`). This will be useful in a subsequent commit to look for and use the locally built `zone.js` package (from `dist/zone.js-dist/`). PR Close #35780 --- aio/tools/ng-packages-installer/index.js | 72 +++++++++++-------- aio/tools/ng-packages-installer/index.spec.js | 23 +++--- 2 files changed, 54 insertions(+), 41 deletions(-) diff --git a/aio/tools/ng-packages-installer/index.js b/aio/tools/ng-packages-installer/index.js index 8db9a480af..46249c18ad 100644 --- a/aio/tools/ng-packages-installer/index.js +++ b/aio/tools/ng-packages-installer/index.js @@ -11,12 +11,11 @@ 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$/; const ANGULAR_ROOT_DIR = path.resolve(__dirname, '../../..'); -const ANGULAR_DIST_PACKAGES = path.join(ANGULAR_ROOT_DIR, 'dist/packages-dist'); -const ANGULAR_DIST_PACKAGES_BUILD_SCRIPT = path.join(ANGULAR_ROOT_DIR, 'scripts/build/build-packages-dist.js'); -const ANGULAR_DIST_PACKAGES_BUILD_CMD = `"${process.execPath}" "${ANGULAR_DIST_PACKAGES_BUILD_SCRIPT}"`; +const ANGULAR_DIST_PACKAGES_DIR = path.join(ANGULAR_ROOT_DIR, 'dist/packages-dist'); +const DIST_PACKAGES_BUILD_SCRIPT = path.join(ANGULAR_ROOT_DIR, 'scripts/build/build-packages-dist.js'); +const DIST_PACKAGES_BUILD_CMD = `"${process.execPath}" "${DIST_PACKAGES_BUILD_SCRIPT}"`; /** * A tool that can install Angular dependencies for a project from NPM or from the @@ -94,7 +93,7 @@ class NgPackagesInstaller { const pkg2 = packages[key2]; if (pkg2) { // point the core Angular packages at the distributable folder - deps[key2] = `file:${pkg2.parentDir}/${key2.replace('@angular/', '')}`; + deps[key2] = `file:${pkg2.packageDir}`; this._log(`Overriding dependency of local ${key} with local package: ${key2}: ${deps[key2]}`); } }); @@ -178,14 +177,14 @@ class NgPackagesInstaller { const canBuild = process.platform !== 'win32'; if (canBuild) { - this._log(`Building the Angular packages with: ${ANGULAR_DIST_PACKAGES_BUILD_SCRIPT}`); - shelljs.exec(ANGULAR_DIST_PACKAGES_BUILD_CMD); + this._log(`Building the Angular packages with: ${DIST_PACKAGES_BUILD_SCRIPT}`); + shelljs.exec(DIST_PACKAGES_BUILD_CMD); } else { this._warn([ 'Automatically building the local Angular packages is currently not supported on Windows.', - `Please, ensure '${ANGULAR_DIST_PACKAGES}' exists and is up-to-date (e.g. by running ` + - `'${ANGULAR_DIST_PACKAGES_BUILD_SCRIPT}' in Git Bash for Windows, Windows Subsystem for Linux or a Linux ` + - 'docker container or VM).', + `Please, ensure '${ANGULAR_DIST_PACKAGES_DIR}' exists and is up-to-date (e.g. by running ` + + `'${DIST_PACKAGES_BUILD_SCRIPT}' in Git Bash for Windows, Windows Subsystem for Linux or a Linux docker ` + + 'container or VM).', '', 'Proceeding anyway...', ].join('\n')); @@ -200,7 +199,7 @@ class NgPackagesInstaller { const sourcePackage = packages[key]; if (sourcePackage) { // point the core Angular packages at the distributable folder - mergedDependencies[key] = `file:${sourcePackage.parentDir}/${key.replace('@angular/', '')}`; + mergedDependencies[key] = `file:${sourcePackage.packageDir}`; this._log(`Overriding dependency with local package: ${key}: ${mergedDependencies[key]}`); // grab peer dependencies const sourcePackagePeerDeps = sourcePackage.config.peerDependencies || {}; @@ -219,32 +218,43 @@ class NgPackagesInstaller { * (Detected as directories in '/dist/packages-dist/' that contain a top-level 'package.json' file.) */ _getDistPackages() { - const packageConfigs = Object.create(null); - const distDir = ANGULAR_DIST_PACKAGES; - - this._log(`Angular distributable directory: ${distDir}.`); + this._log(`Angular distributable directory: ${ANGULAR_DIST_PACKAGES_DIR}.`); if (this.buildPackages) { this._buildDistPackages(); } - shelljs - .find(distDir) - .map(filePath => filePath.slice(distDir.length + 1)) - .filter(filePath => PACKAGE_JSON_REGEX.test(filePath)) - .forEach(packagePath => { - const packageName = `@angular/${packagePath.slice(0, -PACKAGE_JSON.length -1)}`; - if (this.ignorePackages.indexOf(packageName) === -1) { - const packageConfig = require(path.resolve(distDir, packagePath)); - packageConfigs[packageName] = { - parentDir: distDir, - packageJsonPath: path.resolve(distDir, packagePath), - config: packageConfig - }; - } else { - this._log('Ignoring package', packageName); + const collectPackages = containingDir => { + const packages = {}; + + for (const dirName of shelljs.ls(containingDir)) { + const packageDir = path.resolve(containingDir, dirName); + const packageJsonPath = path.join(packageDir, PACKAGE_JSON); + const packageConfig = fs.existsSync(packageJsonPath) ? require(packageJsonPath) : null; + const packageName = packageConfig && packageConfig.name; + + if (!packageConfig) { + // No `package.json` found - this directory is not a package. + continue; + } else if (!packageName) { + // No `name` property in `package.json`. (This should never happen.) + throw new Error(`Package '${packageDir}' specifies no name in its '${PACKAGE_JSON}'.`); + } else if (this.ignorePackages.includes(packageName)) { + this._log(`Ignoring package '${packageName}'.`); + continue; } - }); + + packages[packageName] = { + packageDir, + packageJsonPath, + config: packageConfig, + }; + } + + return packages; + }; + + const packageConfigs = collectPackages(ANGULAR_DIST_PACKAGES_DIR); this._log('Found the following Angular distributables:', Object.keys(packageConfigs).map(key => `\n - ${key}`)); return packageConfigs; diff --git a/aio/tools/ng-packages-installer/index.spec.js b/aio/tools/ng-packages-installer/index.spec.js index 04cfb9f52e..3878859d3c 100644 --- a/aio/tools/ng-packages-installer/index.spec.js +++ b/aio/tools/ng-packages-installer/index.spec.js @@ -66,7 +66,7 @@ describe('NgPackagesInstaller', () => { // These are the packages that are "found" in the dist directory dummyNgPackages = { '@angular/core': { - parentDir: packagesDir, + packageDir: `${packagesDir}/core`, packageJsonPath: `${packagesDir}/core/package.json`, config: { peerDependencies: { @@ -77,17 +77,17 @@ describe('NgPackagesInstaller', () => { } }, '@angular/common': { - parentDir: packagesDir, + packageDir: `${packagesDir}/common`, packageJsonPath: `${packagesDir}/common/package.json`, config: { peerDependencies: { '@angular/core': '4.4.4-1ab23cd4' } } }, '@angular/compiler': { - parentDir: packagesDir, + packageDir: `${packagesDir}/compiler`, packageJsonPath: `${packagesDir}/compiler/package.json`, config: { peerDependencies: { '@angular/common': '4.4.4-1ab23cd4' } } }, '@angular/compiler-cli': { - parentDir: toolsDir, + packageDir: `${toolsDir}/compiler-cli`, packageJsonPath: `${toolsDir}/compiler-cli/package.json`, config: { dependencies: { '@angular/tsc-wrapped': '4.4.4-1ab23cd4' }, @@ -95,7 +95,7 @@ describe('NgPackagesInstaller', () => { } }, '@angular/tsc-wrapped': { - parentDir: toolsDir, + packageDir: `${toolsDir}/tsc-wrapped`, packageJsonPath: `${toolsDir}/tsc-wrapped/package.json`, config: { devDependencies: { '@angular/common': '4.4.4-1ab23cd4' }, @@ -288,7 +288,10 @@ describe('NgPackagesInstaller', () => { }); describe('_getDistPackages()', () => { - beforeEach(() => spyOn(NgPackagesInstaller.prototype, '_buildDistPackages')); + beforeEach(() => { + fs.existsSync.and.callThrough(); + spyOn(NgPackagesInstaller.prototype, '_buildDistPackages'); + }); it('should not build the local packages by default', () => { installer._getDistPackages(); @@ -309,7 +312,7 @@ describe('NgPackagesInstaller', () => { it('should include top level Angular packages', () => { const ngPackages = installer._getDistPackages(); const expectedValue = jasmine.objectContaining({ - parentDir: jasmine.any(String), + packageDir: jasmine.any(String), packageJsonPath: jasmine.any(String), config: jasmine.any(Object), }); @@ -323,12 +326,12 @@ describe('NgPackagesInstaller', () => { expect(ngPackages['@angular/upgrade/static']).not.toBeDefined(); }); - it('should store each package\'s parent directory', () => { + it('should store each package\'s directory', () => { const ngPackages = installer._getDistPackages(); // For example... - expect(ngPackages['@angular/core'].parentDir).toBe(packagesDir); - expect(ngPackages['@angular/router'].parentDir).toBeDefined(toolsDir); + expect(ngPackages['@angular/core'].packageDir).toBe(path.join(packagesDir, 'core')); + expect(ngPackages['@angular/router'].packageDir).toBe(path.join(packagesDir, 'router')); }); it('should not include packages that have been ignored', () => {