fix: pacote does not respect CLI's proxy configuration

Pacote does not respect the proxy settings set with `tns proxy set ...`. Pass the required options and add unit tests for the pacote service (use sinon instead of mocking with injector for the `pacote` and `tar` packages as the service is our wrapper for pacote).

Author: rosen-vladimirov

lib/common

@@ -18,7 +18,7 @@ declare global {
 		extractPackage(packageName: string, destinationDirectory: string, options?: IPacoteExtractOptions): Promise<void>;
 	}
 
-	interface IPacoteBaseOptions {
+	interface IPacoteBaseOptions extends IProxySettingsBase {
 		/**
 		 * The path to npm cache
 		 */

lib/definitions/pacote-service.d.ts

@@ -4,48 +4,78 @@ import * as path from "path";
 
 export class PacoteService implements IPacoteService {
 	constructor(private $fs: IFileSystem,
-		private $npm: INodePackageManager) { }
+		private $npm: INodePackageManager,
+		private $proxyService: IProxyService,
+		private $logger: ILogger) { }
 
 	public async manifest(packageName: string, options?: IPacoteManifestOptions): Promise<any> {
-		// In case `tns create myapp --template https://github.com/NativeScript/template-hello-world.git` command is executed, pacote module throws an error if cache option is not provided.
-		const cache = await this.$npm.getCachePath();
-		const manifestOptions = { cache };
+		this.$logger.trace(`Calling pacoteService.manifest for packageName: '${packageName}' and options: ${options}`);
+		const manifestOptions: IPacoteBaseOptions = await this.getPacoteBaseOptions();
 		if (options) {
 			_.extend(manifestOptions, options);
 		}
 
-		if (this.$fs.exists(packageName)) {
-			packageName = path.resolve(packageName);
-		}
-
+		packageName = this.getRealPackageName(packageName);
+		this.$logger.trace(`Calling pacote.manifest for packageName: ${packageName} and options: ${JSON.stringify(manifestOptions, null, 2)}`);
 		return pacote.manifest(packageName, manifestOptions);
 	}
 
 	public async extractPackage(packageName: string, destinationDirectory: string, options?: IPacoteExtractOptions): Promise<void> {
 		// strip: Remove the specified number of leading path elements. Pathnames with fewer elements will be silently skipped. More info: https://github.com/npm/node-tar/blob/e89c4d37519b1c20133a9f49d5f6b85fa34c203b/README.md
 		// C: Create an archive
+		this.$logger.trace(`Calling pacoteService.extractPackage for packageName: '${packageName}', destinationDir: '${destinationDirectory}' and options: ${options}`);
 		const extractOptions = { strip: 1, C: destinationDirectory };
 		if (options) {
 			_.extend(extractOptions, options);
 		}
 
-		if (this.$fs.exists(packageName)) {
-			packageName = path.resolve(packageName);
-		}
+		packageName = this.getRealPackageName(packageName);
+		const pacoteOptions = await this.getPacoteBaseOptions();
 
-		const cache = await this.$npm.getCachePath();
 		return new Promise<void>((resolve, reject) => {
-			const source = pacote.tarball.stream(packageName, { cache });
+			this.$logger.trace(`Calling pacoteService.extractPackage for packageName: '${packageName}', destinationDir: '${destinationDirectory}' and options: ${options}`);
+
+			const source = pacote.tarball.stream(packageName, pacoteOptions);
 			source.on("error", (err: Error) => {
+				this.$logger.trace(`Error in source while trying to extract stream from ${packageName}. Error is ${err}`);
 				reject(err);
 			});
 
+			this.$logger.trace(`Creating extract tar stream with options: ${JSON.stringify(extractOptions, null, 2)}`);
 			const destination = tar.x(extractOptions);
 			source.pipe(destination);
 
-			destination.on("error", (err: Error) => reject(err));
-			destination.on("finish", () => resolve());
+			destination.on("error", (err: Error) => {
+				this.$logger.trace(`Error in destination while trying to extract stream from ${packageName}. Error is ${err}`);
+				reject(err);
+			});
+
+			destination.on("finish", () => {
+				this.$logger.trace(`Successfully extracted '${packageName}' to ${destinationDirectory}`);
+				resolve();
+			});
 		});
 	}
+
+	private async getPacoteBaseOptions(): Promise<IPacoteBaseOptions> {
+		// In case `tns create myapp --template https://github.com/NativeScript/template-hello-world.git` command is executed, pacote module throws an error if cache option is not provided.
+		const cache = await this.$npm.getCachePath();
+		const pacoteOptions = { cache };
+		const proxySettings = await this.$proxyService.getCache();
+		if (proxySettings) {
+			_.extend(pacoteOptions, proxySettings);
+		}
+
+		return pacoteOptions;
+	}
+
+	private getRealPackageName(packageName: string): string {
+		if (this.$fs.exists(packageName)) {
+			this.$logger.trace(`Will resolve the full path to package ${packageName}.`);
+			packageName = path.resolve(packageName);
+		}
+
+		return packageName;
+	}
 }
 $injector.register("pacoteService", PacoteService);

lib/services/pacote-service.ts

@@ -152,6 +152,9 @@ class ProjectIntegrationTest {
 			executeAfterHooks: async (commandName: string, hookArguments?: IDictionary<any>): Promise<void> => undefined
 		});
 		this.testInjector.register("pacoteService", PacoteService);
+		this.testInjector.register("proxyService", {
+			getCache: async (): Promise<IProxySettings> => null
+		});
 	}
 }
 

test/project-service.ts

@@ -0,0 +1,282 @@
+import { Yok } from "../../lib/common/yok";
+import { assert } from "chai";
+import { PacoteService } from '../../lib/services/pacote-service';
+import { LoggerStub } from "../stubs";
+import { sandbox, SinonSandbox, SinonStub } from "sinon";
+import { EventEmitter } from "events";
+const pacote = require("pacote");
+const tar = require("tar");
+const path = require("path");
+
+const npmCachePath = "npmCachePath";
+const packageName = "testPackage";
+const fullPath = `/Users/username/${packageName}`;
+const destinationDir = "destinationDir";
+const defaultPacoteOpts: IPacoteBaseOptions = { cache: npmCachePath };
+const errorMessage = "error message";
+const proxySettings: IProxySettings = {
+	hostname: "hostname",
+	proxy: "proxy",
+	port: "8888",
+	rejectUnauthorized: true,
+	username: null,
+	password: null
+};
+
+interface ITestSetup {
+	isLocalPackage?: boolean;
+	useProxySettings?: boolean;
+	npmGetCachePathError?: Error;
+}
+
+interface ITestCase extends ITestSetup {
+	manifestOptions?: IPacoteManifestOptions;
+	additionalExtractOpts?: IPacoteExtractOptions;
+	name: string;
+	expectedArgs: any[];
+}
+
+const createTestInjector = (opts?: ITestSetup): IInjector => {
+	opts = opts || {};
+
+	const testInjector = new Yok();
+	testInjector.register("logger", LoggerStub);
+	testInjector.register("pacoteService", PacoteService);
+	testInjector.register("fs", {
+		exists: (p: string): boolean => opts.isLocalPackage
+	});
+
+	testInjector.register("proxyService", {
+		getCache: async (): Promise<IProxySettings> => opts.useProxySettings ? proxySettings : null
+	});
+
+	testInjector.register("npm", {
+		getCachePath: async (): Promise<string> => {
+			if (opts.npmGetCachePathError) {
+				throw opts.npmGetCachePathError;
+			}
+
+			return npmCachePath;
+		}
+	});
+
+	return testInjector;
+};
+
+class MockStream extends EventEmitter {
+	public pipe(destination: any, options?: { end?: boolean; }): any {
+		// Nothing to do here, just mock the method.
+	}
+}
+
+describe("pacoteService", () => {
+	const manifestResult: any = {};
+	const manifestOptions: IPacoteManifestOptions = { fullMetadata: true };
+	let sandboxInstance: SinonSandbox = null;
+	let manifestStub: SinonStub = null;
+	let tarballStreamStub: SinonStub = null;
+	let tarXStub: SinonStub = null;
+	let tarballSourceStream: MockStream = null;
+	let tarExtractDestinationStream: MockStream = null;
+
+	beforeEach(() => {
+		sandboxInstance = sandbox.create();
+		manifestStub = sandboxInstance.stub(pacote, "manifest").returns(Promise.resolve(manifestResult));
+		tarballSourceStream = new MockStream();
+		tarballStreamStub = sandboxInstance.stub(pacote.tarball, "stream").returns(tarballSourceStream);
+		tarExtractDestinationStream = new MockStream();
+		tarXStub = sandboxInstance.stub(tar, "x").returns(tarExtractDestinationStream);
+	});
+
+	afterEach(() => {
+		sandboxInstance.restore();
+	});
+
+	const setupTest = (opts?: ITestSetup): IPacoteService => {
+		opts = opts || {};
+		const testInjector = createTestInjector(opts);
+		if (opts.isLocalPackage) {
+			sandboxInstance.stub(path, "resolve").withArgs(packageName).returns(fullPath);
+		}
+
+		return testInjector.resolve<IPacoteService>("pacoteService");
+	};
+
+	describe("manifest", () => {
+		describe("calls pacote.manifest", () => {
+
+			const testData: ITestCase[] = [
+				{
+					name: "with 'cache' only when no opts are passed",
+					expectedArgs: [packageName, defaultPacoteOpts]
+				},
+				{
+					name: "with 'cache' and passed options",
+					manifestOptions,
+					expectedArgs: [packageName, _.extend({}, defaultPacoteOpts, manifestOptions)]
+				},
+				{
+					name: "with 'cache' and proxy settings",
+					useProxySettings: true,
+					expectedArgs: [packageName, _.extend({}, defaultPacoteOpts, proxySettings)]
+				},
+				{
+					name: "with 'cache', passed options and proxy settings when proxy is configured",
+					manifestOptions,
+					useProxySettings: true,
+					expectedArgs: [packageName, _.extend({}, defaultPacoteOpts, manifestOptions, proxySettings)]
+				},
+				{
+					name: "with full path to file when it is local one",
+					isLocalPackage: true,
+					expectedArgs: [fullPath, defaultPacoteOpts]
+				},
+				{
+					name: "with full path to file, 'cache' and passed options when local path is passed",
+					manifestOptions,
+					isLocalPackage: true,
+					expectedArgs: [fullPath, _.extend({}, defaultPacoteOpts, manifestOptions)]
+				},
+				{
+					name: "with full path to file, 'cache' and proxy settings when proxy is configured",
+					manifestOptions,
+					isLocalPackage: true,
+					useProxySettings: true,
+					expectedArgs: [fullPath, _.extend({}, defaultPacoteOpts, manifestOptions, proxySettings)]
+				},
+				{
+					name: "with full path to file, 'cache', passed options and proxy settings when proxy is configured and local path is passed",
+					manifestOptions,
+					useProxySettings: true,
+					isLocalPackage: true,
+					expectedArgs: [fullPath, _.extend({}, defaultPacoteOpts, manifestOptions, proxySettings)]
+				},
+			];
+
+			testData.forEach(testCase => {
+				it(testCase.name, async () => {
+					const pacoteService = setupTest(testCase);
+					const result = await pacoteService.manifest(packageName, testCase.manifestOptions);
+
+					assert.equal(result, manifestResult);
+					assert.deepEqual(manifestStub.firstCall.args, testCase.expectedArgs);
+				});
+			});
+		});
+
+		it("fails with npm error when unable to get npm cache", async () => {
+			const npmGetCachePathError = new Error("npm error");
+			const pacoteService = setupTest({ npmGetCachePathError });
+			await assert.isRejected(pacoteService.manifest(packageName, null), npmGetCachePathError.message);
+		});
+	});
+
+	describe("extractPackage", () => {
+		it("fails with correct error when pacote.tarball.stream raises error event", async () => {
+			const pacoteService = setupTest();
+
+			const pacoteExtractPackagePromise = pacoteService.extractPackage(packageName, destinationDir);
+			setImmediate(() => {
+				tarballSourceStream.emit("error", new Error(errorMessage));
+			});
+
+			await assert.isRejected(pacoteExtractPackagePromise, errorMessage);
+		});
+
+		it("fails with correct error when the destination stream raises error event", async () => {
+			const pacoteService = setupTest();
+
+			const pacoteExtractPackagePromise = pacoteService.extractPackage(packageName, destinationDir);
+			setImmediate(() => {
+				tarExtractDestinationStream.emit("error", new Error(errorMessage));
+			});
+
+			await assert.isRejected(pacoteExtractPackagePromise, errorMessage);
+		});
+
+		it("resolves when the destination stream emits finish event", async () => {
+			const pacoteService = setupTest();
+
+			const pacoteExtractPackagePromise = pacoteService.extractPackage(packageName, destinationDir);
+			setImmediate(() => {
+				tarExtractDestinationStream.emit("finish");
+			});
+
+			await assert.isFulfilled(pacoteExtractPackagePromise);
+		});
+
+		describe("passes correct options to tar.x", () => {
+			const defaultExtractOpts = { strip: 1, C: destinationDir };
+			const additionalExtractOpts: IPacoteExtractOptions = {
+				filter: (p: string, stat: any) => true
+			};
+
+			const testData: ITestCase[] = [
+				{
+					name: "when only default options should be passed",
+					expectedArgs: [defaultExtractOpts],
+				},
+				{
+					name: "when additional options are passed",
+					expectedArgs: [_.extend({}, defaultExtractOpts, additionalExtractOpts)],
+					additionalExtractOpts
+				},
+			];
+
+			testData.forEach(testCase => {
+				it(testCase.name, async () => {
+					const pacoteService = setupTest();
+
+					const pacoteExtractPackagePromise = pacoteService.extractPackage(packageName, destinationDir, testCase.additionalExtractOpts);
+					setImmediate(() => {
+						tarExtractDestinationStream.emit("finish");
+					});
+
+					await assert.isFulfilled(pacoteExtractPackagePromise);
+
+					assert.deepEqual(tarXStub.firstCall.args, testCase.expectedArgs);
+				});
+			});
+		});
+
+		describe("passes correct options to pacote.tarball.stream", () => {
+			const testData: ITestCase[] = [
+				{
+					name: "when proxy is not set",
+					expectedArgs: [packageName, defaultPacoteOpts]
+				},
+				{
+					name: "when proxy is not set and a local path is passed",
+					isLocalPackage: true,
+					expectedArgs: [fullPath, defaultPacoteOpts]
+				},
+				{
+					name: "when proxy is set",
+					useProxySettings: true,
+					expectedArgs: [packageName, _.extend({}, defaultPacoteOpts, proxySettings)]
+				},
+				{
+					name: "when proxy is set and a local path is passed",
+					useProxySettings: true,
+					isLocalPackage: true,
+					expectedArgs: [fullPath, _.extend({}, defaultPacoteOpts, proxySettings)]
+				},
+
+			];
+
+			testData.forEach(testCase => {
+				it(testCase.name, async () => {
+					const pacoteService = setupTest(testCase);
+
+					const pacoteExtractPackagePromise = pacoteService.extractPackage(packageName, destinationDir);
+					setImmediate(() => {
+						tarExtractDestinationStream.emit("finish");
+					});
+
+					await assert.isFulfilled(pacoteExtractPackagePromise);
+					assert.deepEqual(tarballStreamStub.firstCall.args, testCase.expectedArgs);
+				});
+			});
+		});
+	});
+});