Merge branch 'feat/pkg-mngr-impl' into refactor/pkg-mgr-replace

msc-ddiaz · msc-ddiaz · commit 80f2d6449c00 · 2018-10-23T10:14:48.000-07:00
diff --git a/lib/base-package-manager.ts b/lib/base-package-manager.ts
@@ -1,74 +1,26 @@
-import * as child_process from 'child_process';
+import { isInteractive } from "./common/helpers";
 
 export class BasePackageManager {
-	constructor(private packageManager: string) { }
+	constructor(
+		protected $childProcess: IChildProcess,
+		private $hostInfo: IHostInfo,
+		private packageManager: string
+	) { }
 
-	protected getNpmExecutableName(isWindows: boolean): string {
+	protected getPackageManagerExecutableName(): string {
 		let npmExecutableName = this.packageManager;
 
-		if (isWindows) {
+		if (this.$hostInfo.isWindows) {
 			npmExecutableName += ".cmd";
 		}
 
 		return npmExecutableName;
 	}
 
-	protected async processPackageManagerInstall(
-		childProcess: child_process.ChildProcess,
-		isWindows: boolean,
-		params: string[],
-	): Promise<ISpawnResult> {
-		return new Promise<ISpawnResult>((resolve, reject) => {
-			let isFulfilled = false;
-			let capturedOut = "";
-			let capturedErr = "";
-
-			const npmExecutable = this.getNpmExecutableName(isWindows);
-
-			if (childProcess.stdout) {
-				childProcess.stdout.on("data", (data: string) => {
-					capturedOut += data;
-				});
-			}
-
-			if (childProcess.stderr) {
-				childProcess.stderr.on("data", (data: string) => {
-					capturedErr += data;
-				});
-			}
-
-			childProcess.on("close", (arg: any) => {
-				const exitCode = typeof arg === "number" ? arg : arg && arg.code;
-
-				if (exitCode === 0) {
-					isFulfilled = true;
-					const result = {
-						stdout: capturedOut,
-						stderr: capturedErr,
-						exitCode
-					};
-
-					resolve(result);
-				} else {
-					let errorMessage = `Command ${npmExecutable} ${params && params.join(" ")} failed with exit code ${exitCode}`;
-					if (capturedErr) {
-						errorMessage += ` Error output: \n ${capturedErr}`;
-					}
-
-					if (!isFulfilled) {
-						isFulfilled = true;
-						reject(new Error(errorMessage));
-					}
-				}
-			});
-
-			childProcess.on("error", (err: Error) => {
-				if (!isFulfilled) {
-					isFulfilled = true;
-					reject(err);
-				}
-			});
-		});
+	protected async processPackageManagerInstall(params: string[], opts: { cwd: string }) {
+		const npmExecutable = this.getPackageManagerExecutableName();
+		const stdioValue = isInteractive() ? "inherit" : "pipe";
+		return await this.$childProcess.spawnFromEvent(npmExecutable, params, "close", { cwd: opts.cwd, stdio: stdioValue });
 	}
 
 	protected getFlagsString(config: any, asArray: boolean): any {
diff --git a/lib/node-package-manager.ts b/lib/node-package-manager.ts
@@ -1,20 +1,20 @@
 import * as path from "path";
 import { BasePackageManager } from "./base-package-manager";
 import { exported, cache } from "./common/decorators";
-import { isInteractive } from "./common/helpers";
 import { CACACHE_DIRECTORY_NAME } from "./constants";
 
 export class NodePackageManager extends BasePackageManager implements INodePackageManager {
 	private static SCOPED_DEPENDENCY_REGEXP = /^(@.+?)(?:@(.+?))?$/;
 	private static DEPENDENCY_REGEXP = /^(.+?)(?:@(.+?))?$/;
 
-	constructor(private $fs: IFileSystem,
-		private $hostInfo: IHostInfo,
+	constructor(
+		$childProcess: IChildProcess,
 		private $errors: IErrors,
-		private $childProcess: IChildProcess,
+		private $fs: IFileSystem,
+		$hostInfo: IHostInfo,
 		private $logger: ILogger,
 		private $httpClient: Server.IHttpClient) {
-		super('npm');
+		super($childProcess, $hostInfo, 'npm');
 	}
 
 	@exported("npm")
@@ -56,7 +56,7 @@ export class NodePackageManager extends BasePackageManager implements INodePacka
 		}
 
 		try {
-			const spawnResult: ISpawnResult = await this.getNpmInstallResult(params, cwd);
+			const spawnResult: ISpawnResult = await this.processPackageManagerInstall(params, { cwd });
 
 			// Whenever calling npm install without any arguments (hence installing all dependencies) no output is emitted on stdout
 			// Luckily, whenever you call npm install to install all dependencies chances are you won't need the name/version of the package you're installing because there is none.
@@ -69,7 +69,7 @@ export class NodePackageManager extends BasePackageManager implements INodePacka
 			// We cannot use the actual install with --json to get the information because of post-install scripts which may print on stdout
 			// dry-run install is quite fast when the dependencies are already installed even for many dependencies (e.g. angular) so we can live with this approach
 			// We need the --prefix here because without it no output is emitted on stdout because all the dependencies are already installed.
-			const spawnNpmDryRunResult = await this.$childProcess.spawnFromEvent(this.getNpmExecutableName(this.$hostInfo.isWindows), params, "close");
+			const spawnNpmDryRunResult = await this.$childProcess.spawnFromEvent(this.getPackageManagerExecutableName(), params, "close");
 			return this.parseNpmInstallResult(spawnNpmDryRunResult.stdout, spawnResult.stdout, packageName);
 		} catch (err) {
 			if (err.message && err.message.indexOf("EPEERINVALID") !== -1) {
@@ -211,21 +211,6 @@ export class NodePackageManager extends BasePackageManager implements INodePacka
 			version
 		};
 	}
-
-	private async getNpmInstallResult(params: string[], cwd: string): Promise<ISpawnResult> {
-		return new Promise<ISpawnResult>(async (resolve, reject) => {
-			const npmExecutable = this.getNpmExecutableName(this.$hostInfo.isWindows);
-			const stdioValue = isInteractive() ? "inherit" : "pipe";
-			const childProcess = this.$childProcess.spawn(npmExecutable, params, { cwd, stdio: stdioValue });
-
-			try {
-				const result = await this.processPackageManagerInstall(childProcess, this.$hostInfo.isWindows, params);
-				resolve(result);
-			} catch (e) {
-				reject(e);
-			}
-		});
-	}
 }
 
 $injector.register("npm", NodePackageManager);
diff --git a/lib/package-manager.ts b/lib/package-manager.ts
@@ -39,13 +39,15 @@ export class PackageManager implements INodePackageManager {
 	}
 
 	private _determinePackageManager(): void {
-		this.$userSettingsService.getSettingValue('packageManager').then ( (pm: string) => {
+		this.$userSettingsService.getSettingValue('packageManager')
+		.then((pm: string) => {
 			if (pm === 'yarn' || this.$options.yarn) {
 				this.packageManager = this.$yarn;
 			} else {
 				this.packageManager = this.$npm;
 			}
-		}, (err) => {
+		})
+		.catch((err) => {
 			this.$errors.fail(`Unable to read package manager config from user settings ${err}`);
 		});
 	}
diff --git a/lib/yarn-package-manager.ts b/lib/yarn-package-manager.ts
@@ -1,20 +1,19 @@
 import * as path from "path";
 import { BasePackageManager } from "./base-package-manager";
 import { exported } from './common/decorators';
-import { isInteractive } from "./common/helpers";
 
 export class YarnPackageManager extends BasePackageManager implements INodePackageManager {
 
 	constructor(
-		private $childProcess: IChildProcess,
+		$childProcess: IChildProcess,
 		private $errors: IErrors,
 		private $fs: IFileSystem,
-		private $hostInfo: IHostInfo,
+		$hostInfo: IHostInfo,
 		private $httpClient: Server.IHttpClient,
 		private $logger: ILogger,
 		private $pacoteService: IPacoteService
 	) {
-		super('yarn');
+		super($childProcess, $hostInfo, 'yarn');
 	}
 
 	@exported("yarn")
@@ -40,7 +39,7 @@ export class YarnPackageManager extends BasePackageManager implements INodePacka
 		const cwd = pathToSave;
 
 		try {
-			await this._getYarnInstallResult(params, cwd);
+			await this.processPackageManagerInstall(params, { cwd });
 
 			if (isInstallingAllDependencies) {
 				return null;
@@ -92,7 +91,7 @@ export class YarnPackageManager extends BasePackageManager implements INodePacka
 	@exported("yarn")
 	public async getRegistryPackageData(packageName: string): Promise<any> {
 		const registry = await this.$childProcess.exec(`yarn config get registry`);
-		const url =  registry.trim() + packageName;
+		const url = `${registry.trim()}/${packageName}`;
 		this.$logger.trace(`Trying to get data from yarn registry for package ${packageName}, url is: ${url}`);
 		const responseData = (await this.$httpClient.httpRequest(url)).body;
 		this.$logger.trace(`Successfully received data from yarn registry for package ${packageName}. Response data is: ${responseData}`);
@@ -101,21 +100,6 @@ export class YarnPackageManager extends BasePackageManager implements INodePacka
 		return jsonData;
 	}
 
-	private async _getYarnInstallResult(params: string[], cwd: string): Promise<ISpawnResult> {
-		return new Promise<ISpawnResult>(async (resolve, reject) => {
-			const npmExecutable = this.getNpmExecutableName(this.$hostInfo.isWindows);
-			const stdioValue = isInteractive() ? "inherit" : "pipe";
-			const childProcess = this.$childProcess.spawn(npmExecutable, params, { cwd, stdio: stdioValue });
-			try {
-				await this.processPackageManagerInstall(childProcess, this.$hostInfo.isWindows, params);
-				resolve();
-			} catch (e) {
-				reject(e);
-			}
-
-		});
-	}
-
 	@exported("yarn")
 	getCachePath(): Promise<string> {
 		throw new Error("Method not implemented.");

Original file line number	Diff line number	Diff line change
`@@ -39,13 +39,15 @@ export class PackageManager implements INodePackageManager {`
`39`	`39`	`}`
`40`	`40`
`41`	`41`	`private _determinePackageManager(): void {`
`42`		`- this.$userSettingsService.getSettingValue('packageManager').then ( (pm: string) => {`
	`42`	`+ this.$userSettingsService.getSettingValue('packageManager')`
	`43`	`+ .then((pm: string) => {`
`43`	`44`	`if (pm === 'yarn' \|\| this.$options.yarn) {`
`44`	`45`	`this.packageManager = this.$yarn;`
`45`	`46`	`} else {`
`46`	`47`	`this.packageManager = this.$npm;`
`47`	`48`	`}`
`48`		`- }, (err) => {`
	`49`	`+ })`
	`50`	`+ .catch((err) => {`
`49`	`51`	this.$errors.fail(`Unable to read package manager config from user settings ${err}`);
`50`	`52`	`});`
`51`	`53`	`}`