Skip to content

Commit 63fd629

Browse files
rosen-vladimirovrosen-vladimirov
committed
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).
1 parent a3ebf5d commit 63fd629

File tree

5 files changed

+322
-10
lines changed

5 files changed

+322
-10
lines changed

‎lib/common

‎lib/definitions/pacote-service.d.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ declare global {
1818
extractPackage(packageName: string, destinationDirectory: string, options?: IPacoteExtractOptions): Promise<void>;
1919
}
2020

21-
interface IPacoteBaseOptions {
21+
interface IPacoteBaseOptions extends IProxySettingsBase {
2222
/**
2323
* The path to npm cache
2424
*/

‎lib/services/pacote-service.ts

+36-8
Original file line numberDiff line numberDiff line change
@@ -4,47 +4,75 @@ import * as path from "path";
44

55
export class PacoteService implements IPacoteService {
66
constructor(private $fs: IFileSystem,
7-
private $npm: INodePackageManager) { }
7+
private $npm: INodePackageManager,
8+
private $proxyService: IProxyService,
9+
private $logger: ILogger) { }
810

911
public async manifest(packageName: string, options?: IPacoteManifestOptions): Promise<any> {
10-
// 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.
11-
const cache = await this.$npm.getCachePath();
12-
const manifestOptions = { cache };
12+
this.$logger.trace(`Calling pacoteService.manifest for packageName: '${packageName}' and options: ${options}`);
13+
const manifestOptions: IPacoteBaseOptions = await this.getPacoteBaseOptions();
1314
if (options) {
1415
_.extend(manifestOptions, options);
1516
}
1617

1718
if (this.$fs.exists(packageName)) {
19+
this.$logger.trace(`Will resolve the full path to package ${packageName}.`);
1820
packageName = path.resolve(packageName);
1921
}
2022

23+
this.$logger.trace(`Calling pacote.manifest for packageName: ${packageName} and options: ${JSON.stringify(manifestOptions, null, 2)}`);
2124
return pacote.manifest(packageName, manifestOptions);
2225
}
2326

27+
private async getPacoteBaseOptions(): Promise<IPacoteBaseOptions> {
28+
// 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.
29+
const cache = await this.$npm.getCachePath();
30+
const pacoteOptions = { cache };
31+
const proxySettings = await this.$proxyService.getCache();
32+
if (proxySettings) {
33+
_.extend(pacoteOptions, proxySettings);
34+
}
35+
36+
return pacoteOptions;
37+
}
38+
2439
public async extractPackage(packageName: string, destinationDirectory: string, options?: IPacoteExtractOptions): Promise<void> {
2540
// 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
2641
// C: Create an archive
42+
this.$logger.trace(`Calling pacoteService.extractPackage for packageName: '${packageName}', destinationDir: '${destinationDirectory}' and options: ${options}`);
2743
const extractOptions = { strip: 1, C: destinationDirectory };
2844
if (options) {
2945
_.extend(extractOptions, options);
3046
}
3147

3248
if (this.$fs.exists(packageName)) {
49+
this.$logger.trace(`Will resolve the full path to package ${packageName}.`);
3350
packageName = path.resolve(packageName);
3451
}
3552

36-
const cache = await this.$npm.getCachePath();
53+
const pacoteOptions = await this.getPacoteBaseOptions();
3754
return new Promise<void>((resolve, reject) => {
38-
const source = pacote.tarball.stream(packageName, { cache });
55+
this.$logger.trace(`Calling pacoteService.extractPackage for packageName: '${packageName}', destinationDir: '${destinationDirectory}' and options: ${options}`);
56+
57+
const source = pacote.tarball.stream(packageName, pacoteOptions);
3958
source.on("error", (err: Error) => {
59+
this.$logger.trace(`Error in source while trying to extract stream from ${packageName}. Error is ${err}`);
4060
reject(err);
4161
});
4262

63+
this.$logger.trace(`Creating extract tar stream with options: ${JSON.stringify(extractOptions, null, 2)}`);
4364
const destination = tar.x(extractOptions);
4465
source.pipe(destination);
4566

46-
destination.on("error", (err: Error) => reject(err));
47-
destination.on("finish", () => resolve());
67+
destination.on("error", (err: Error) => {
68+
this.$logger.trace(`Error in destination while trying to extract stream from ${packageName}. Error is ${err}`);
69+
reject(err);
70+
});
71+
72+
destination.on("finish", () => {
73+
this.$logger.trace(`Successfully extracted '${packageName}' to ${destinationDirectory}`);
74+
resolve();
75+
});
4876
});
4977
}
5078
}

‎test/project-service.ts

+3
Original file line numberDiff line numberDiff line change
@@ -152,6 +152,9 @@ class ProjectIntegrationTest {
152152
executeAfterHooks: async (commandName: string, hookArguments?: IDictionary<any>): Promise<void> => undefined
153153
});
154154
this.testInjector.register("pacoteService", PacoteService);
155+
this.testInjector.register("proxyService", {
156+
getCache: async (): Promise<IProxySettings> => null
157+
});
155158
}
156159
}
157160

‎test/services/pacote-service.ts

+281
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,281 @@
1+
import { Yok } from "../../lib/common/yok";
2+
import { assert } from "chai";
3+
import { PacoteService } from '../../lib/services/pacote-service';
4+
import { LoggerStub } from "../stubs";
5+
import { sandbox, SinonSandbox, SinonStub } from "sinon";
6+
import { EventEmitter } from "events";
7+
const pacote = require("pacote");
8+
const tar = require("tar");
9+
const path = require("path");
10+
11+
const npmCachePath = "npmCachePath";
12+
const packageName = "testPackage";
13+
const fullPath = `/Users/username/${packageName}`;
14+
const destinationDir = "destinationDir";
15+
const defaultPacoteOpts: IPacoteBaseOptions = { cache: npmCachePath };
16+
const proxySettings: IProxySettings = {
17+
hostname: "hostname",
18+
proxy: "proxy",
19+
port: "8888",
20+
rejectUnauthorized: true,
21+
username: null,
22+
password: null
23+
};
24+
25+
interface ITestSetup {
26+
isLocalPackage?: boolean;
27+
useProxySettings?: boolean;
28+
npmGetCachePathError?: Error;
29+
}
30+
31+
interface ITestCase extends ITestSetup {
32+
manifestOptions?: IPacoteManifestOptions;
33+
additionalExtractOpts?: IPacoteExtractOptions;
34+
name: string;
35+
expectedArgs: any[];
36+
}
37+
38+
const createTestInjector = (opts?: ITestSetup): IInjector => {
39+
opts = opts || {};
40+
41+
const testInjector = new Yok();
42+
testInjector.register("logger", LoggerStub);
43+
testInjector.register("pacoteService", PacoteService);
44+
testInjector.register("fs", {
45+
exists: (p: string): boolean => opts.isLocalPackage
46+
});
47+
48+
testInjector.register("proxyService", {
49+
getCache: async (): Promise<IProxySettings> => opts.useProxySettings ? proxySettings : null
50+
});
51+
52+
testInjector.register("npm", {
53+
getCachePath: async (): Promise<string> => {
54+
if (opts.npmGetCachePathError) {
55+
throw opts.npmGetCachePathError;
56+
}
57+
58+
return npmCachePath;
59+
}
60+
});
61+
62+
return testInjector;
63+
};
64+
65+
class MockStream extends EventEmitter {
66+
public pipe(destination: any, options?: { end?: boolean; }): any {
67+
// Nothing to do here, just mock the method.
68+
}
69+
}
70+
71+
describe("pacoteService", () => {
72+
const manifestResult: any = {};
73+
const manifestOptions: IPacoteManifestOptions = { fullMetadata: true };
74+
let sandboxInstance: SinonSandbox = null;
75+
let manifestStub: SinonStub = null;
76+
let tarballStreamStub: SinonStub = null;
77+
let tarXStub: SinonStub = null;
78+
let tarballSourceStream: MockStream = null;
79+
let tarExtractDestinationStream: MockStream = null;
80+
81+
beforeEach(() => {
82+
sandboxInstance = sandbox.create();
83+
manifestStub = sandboxInstance.stub(pacote, "manifest").returns(Promise.resolve(manifestResult));
84+
tarballSourceStream = new MockStream();
85+
tarballStreamStub = sandboxInstance.stub(pacote.tarball, "stream").returns(tarballSourceStream);
86+
tarExtractDestinationStream = new MockStream();
87+
tarXStub = sandboxInstance.stub(tar, "x").returns(tarExtractDestinationStream);
88+
});
89+
90+
afterEach(() => {
91+
sandboxInstance.restore();
92+
});
93+
94+
const setupTest = (opts?: ITestSetup): IPacoteService => {
95+
opts = opts || {};
96+
const testInjector = createTestInjector(opts);
97+
if (opts.isLocalPackage) {
98+
sandboxInstance.stub(path, "resolve").withArgs(packageName).returns(fullPath);
99+
}
100+
101+
return testInjector.resolve<IPacoteService>("pacoteService");
102+
};
103+
104+
describe("manifest", () => {
105+
describe("calls pacote.manifest", () => {
106+
107+
const testData: ITestCase[] = [
108+
{
109+
name: "with 'cache' only when no opts are passed",
110+
expectedArgs: [packageName, defaultPacoteOpts]
111+
},
112+
{
113+
name: "with 'cache' and passed options",
114+
manifestOptions,
115+
expectedArgs: [packageName, _.extend({}, defaultPacoteOpts, manifestOptions)]
116+
},
117+
{
118+
name: "with 'cache' and proxy settings",
119+
useProxySettings: true,
120+
expectedArgs: [packageName, _.extend({}, defaultPacoteOpts, proxySettings)]
121+
},
122+
{
123+
name: "with 'cache', passed options and proxy settings when proxy is configured",
124+
manifestOptions,
125+
useProxySettings: true,
126+
expectedArgs: [packageName, _.extend({}, defaultPacoteOpts, manifestOptions, proxySettings)]
127+
},
128+
{
129+
name: "with full path to file when it is local one",
130+
isLocalPackage: true,
131+
expectedArgs: [fullPath, defaultPacoteOpts]
132+
},
133+
{
134+
name: "with full path to file, 'cache' and passed options when local path is passed",
135+
manifestOptions,
136+
isLocalPackage: true,
137+
expectedArgs: [fullPath, _.extend({}, defaultPacoteOpts, manifestOptions)]
138+
},
139+
{
140+
name: "with full path to file, 'cache' and proxy settings when proxy is configured",
141+
manifestOptions,
142+
isLocalPackage: true,
143+
useProxySettings: true,
144+
expectedArgs: [fullPath, _.extend({}, defaultPacoteOpts, manifestOptions, proxySettings)]
145+
},
146+
{
147+
name: "with full path to file, 'cache', passed options and proxy settings when proxy is configured and local path is passed",
148+
manifestOptions,
149+
useProxySettings: true,
150+
isLocalPackage: true,
151+
expectedArgs: [fullPath, _.extend({}, defaultPacoteOpts, manifestOptions, proxySettings)]
152+
},
153+
];
154+
155+
testData.forEach(testCase => {
156+
it(testCase.name, async () => {
157+
const pacoteService = setupTest(testCase);
158+
const result = await pacoteService.manifest(packageName, testCase.manifestOptions);
159+
160+
assert.equal(result, manifestResult);
161+
assert.deepEqual(manifestStub.firstCall.args, testCase.expectedArgs);
162+
});
163+
});
164+
});
165+
166+
it("fails with npm error when unable to get npm cache", async () => {
167+
const npmGetCachePathError = new Error("npm error");
168+
const pacoteService = setupTest({ npmGetCachePathError });
169+
await assert.isRejected(pacoteService.manifest(packageName, null), npmGetCachePathError.message);
170+
});
171+
});
172+
173+
describe("extractPackage", () => {
174+
it("fails with correct error when pacote.tarball.stream raises error event", async () => {
175+
const pacoteService = setupTest();
176+
177+
const pacoteExtractPackagePromise = pacoteService.extractPackage(packageName, destinationDir);
178+
setImmediate(() => {
179+
tarballSourceStream.emit("error", new Error("my error"));
180+
});
181+
182+
await assert.isRejected(pacoteExtractPackagePromise, "my error");
183+
});
184+
185+
it("fails with correct error when the destination stream raises error event", async () => {
186+
const pacoteService = setupTest();
187+
188+
const pacoteExtractPackagePromise = pacoteService.extractPackage(packageName, destinationDir);
189+
setImmediate(() => {
190+
tarExtractDestinationStream.emit("error", new Error("my error"));
191+
});
192+
193+
await assert.isRejected(pacoteExtractPackagePromise, "my error");
194+
});
195+
196+
it("resolves when the destination stream emits finish event", async () => {
197+
const pacoteService = setupTest();
198+
199+
const pacoteExtractPackagePromise = pacoteService.extractPackage(packageName, destinationDir);
200+
setImmediate(() => {
201+
tarExtractDestinationStream.emit("finish");
202+
});
203+
204+
await assert.isFulfilled(pacoteExtractPackagePromise);
205+
});
206+
207+
describe("passes correct options to tar.x", () => {
208+
const defaultExtractOpts = { strip: 1, C: destinationDir };
209+
const additionalExtractOpts: IPacoteExtractOptions = {
210+
filter: (p: string, stat: any) => true
211+
};
212+
213+
const testData: ITestCase[] = [
214+
{
215+
name: "when only default options should be passed",
216+
expectedArgs: [defaultExtractOpts],
217+
},
218+
{
219+
name: "when additional options are passed",
220+
expectedArgs: [_.extend({}, defaultExtractOpts, additionalExtractOpts)],
221+
additionalExtractOpts
222+
},
223+
];
224+
225+
testData.forEach(testCase => {
226+
it(testCase.name, async () => {
227+
const pacoteService = setupTest();
228+
229+
const pacoteExtractPackagePromise = pacoteService.extractPackage(packageName, destinationDir, testCase.additionalExtractOpts);
230+
setImmediate(() => {
231+
tarExtractDestinationStream.emit("finish");
232+
});
233+
234+
await assert.isFulfilled(pacoteExtractPackagePromise);
235+
236+
assert.deepEqual(tarXStub.firstCall.args, testCase.expectedArgs);
237+
});
238+
});
239+
});
240+
241+
describe("passes correct options to pacote.tarball.stream", () => {
242+
const testData: ITestCase[] = [
243+
{
244+
name: "when proxy is not set",
245+
expectedArgs: [packageName, defaultPacoteOpts]
246+
},
247+
{
248+
name: "when proxy is not set and a local path is passed",
249+
isLocalPackage: true,
250+
expectedArgs: [fullPath, defaultPacoteOpts]
251+
},
252+
{
253+
name: "when proxy is set",
254+
useProxySettings: true,
255+
expectedArgs: [packageName, _.extend({}, defaultPacoteOpts, proxySettings)]
256+
},
257+
{
258+
name: "when proxy is set and a local path is passed",
259+
useProxySettings: true,
260+
isLocalPackage: true,
261+
expectedArgs: [fullPath, _.extend({}, defaultPacoteOpts, proxySettings)]
262+
},
263+
264+
];
265+
266+
testData.forEach(testCase => {
267+
it(testCase.name, async () => {
268+
const pacoteService = setupTest(testCase);
269+
270+
const pacoteExtractPackagePromise = pacoteService.extractPackage(packageName, destinationDir);
271+
setImmediate(() => {
272+
tarExtractDestinationStream.emit("finish");
273+
});
274+
275+
await assert.isFulfilled(pacoteExtractPackagePromise);
276+
assert.deepEqual(tarballStreamStub.firstCall.args, testCase.expectedArgs);
277+
});
278+
});
279+
});
280+
});
281+
});

0 commit comments

Comments
 (0)