Skip to content

Commit b2b360a

Browse files
sheetalkamatsheetalkamat
authored
Merge pull request microsoft#29816 from Microsoft/ti
Use execFileSync in typing installer
2 parents d517713 + bc386c1 commit b2b360a

3 files changed

Lines changed: 26 additions & 19 deletions

File tree

‎src/testRunner/unittests/tsserver/typingsInstaller.ts‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1684,19 +1684,19 @@ namespace ts.projectSystem {
16841684
TI.getNpmCommandForInstallation(npmPath, tsVersion, packageNames, packageNames.length - Math.ceil(packageNames.length / 2)).command
16851685
];
16861686
it("works when the command is too long to install all packages at once", () => {
1687-
const commands: string[] = [];
1688-
const hasError = TI.installNpmPackages(npmPath, tsVersion, packageNames, command => {
1689-
commands.push(command);
1687+
const commands: [string, string[]][] = [];
1688+
const hasError = TI.installNpmPackages(npmPath, tsVersion, packageNames, (file, args) => {
1689+
commands.push([file, args]);
16901690
return false;
16911691
});
16921692
assert.isFalse(hasError);
16931693
assert.deepEqual(commands, expectedCommands, "commands");
16941694
});
16951695

16961696
it("installs remaining packages when one of the partial command fails", () => {
1697-
const commands: string[] = [];
1698-
const hasError = TI.installNpmPackages(npmPath, tsVersion, packageNames, command => {
1699-
commands.push(command);
1697+
const commands: [string, string[]][] = [];
1698+
const hasError = TI.installNpmPackages(npmPath, tsVersion, packageNames, (file, args) => {
1699+
commands.push([file, args]);
17001700
return commands.length === 1;
17011701
});
17021702
assert.isTrue(hasError);

‎src/typingsInstaller/nodeTypingsInstaller.ts‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -70,10 +70,10 @@ namespace ts.server.typingsInstaller {
7070
cwd: string;
7171
encoding: "utf-8";
7272
}
73-
type ExecSync = (command: string, options: ExecSyncOptions) => string;
73+
type ExecFileSync = (file: string, args: string[], options: ExecSyncOptions) => string;
7474

7575
export class NodeTypingsInstaller extends TypingsInstaller {
76-
private readonly nodeExecSync: ExecSync;
76+
private readonly nodeExecFileSync: ExecFileSync;
7777
private readonly npmPath: string;
7878
readonly typesRegistry: Map<MapLike<string>>;
7979

@@ -97,15 +97,15 @@ namespace ts.server.typingsInstaller {
9797
this.log.writeLine(`Process id: ${process.pid}`);
9898
this.log.writeLine(`NPM location: ${this.npmPath} (explicit '${Arguments.NpmLocation}' ${npmLocation === undefined ? "not " : ""} provided)`);
9999
}
100-
({ execSync: this.nodeExecSync } = require("child_process"));
100+
({ execFileSync: this.nodeExecFileSync } = require("child_process"));
101101

102102
this.ensurePackageDirectoryExists(globalTypingsCacheLocation);
103103

104104
try {
105105
if (this.log.isEnabled()) {
106106
this.log.writeLine(`Updating ${typesRegistryPackageName} npm package...`);
107107
}
108-
this.execSyncAndLog(`${this.npmPath} install --ignore-scripts ${typesRegistryPackageName}@${this.latestDistTag}`, { cwd: globalTypingsCacheLocation });
108+
this.execFileSyncAndLog(this.npmPath, ["install", "--ignore-scripts", `${typesRegistryPackageName}@${this.latestDistTag}`], { cwd: globalTypingsCacheLocation });
109109
if (this.log.isEnabled()) {
110110
this.log.writeLine(`Updated ${typesRegistryPackageName} npm package`);
111111
}
@@ -189,20 +189,20 @@ namespace ts.server.typingsInstaller {
189189
this.log.writeLine(`#${requestId} with arguments'${JSON.stringify(packageNames)}'.`);
190190
}
191191
const start = Date.now();
192-
const hasError = installNpmPackages(this.npmPath, version, packageNames, command => this.execSyncAndLog(command, { cwd }));
192+
const hasError = installNpmPackages(this.npmPath, version, packageNames, (file, args) => this.execFileSyncAndLog(file, args, { cwd }));
193193
if (this.log.isEnabled()) {
194194
this.log.writeLine(`npm install #${requestId} took: ${Date.now() - start} ms`);
195195
}
196196
onRequestCompleted(!hasError);
197197
}
198198

199199
/** Returns 'true' in case of error. */
200-
private execSyncAndLog(command: string, options: Pick<ExecSyncOptions, "cwd">): boolean {
200+
private execFileSyncAndLog(file: string, args: string[], options: Pick<ExecSyncOptions, "cwd">): boolean {
201201
if (this.log.isEnabled()) {
202-
this.log.writeLine(`Exec: ${command}`);
202+
this.log.writeLine(`Exec: ${file} ${args.join(" ")}`);
203203
}
204204
try {
205-
const stdout = this.nodeExecSync(command, { ...options, encoding: "utf-8" });
205+
const stdout = this.nodeExecFileSync(file, args, { ...options, encoding: "utf-8" });
206206
if (this.log.isEnabled()) {
207207
this.log.writeLine(` Succeeded. stdout:${indent(sys.newLine, stdout)}`);
208208
}

‎src/typingsInstallerCore/typingsInstaller.ts‎

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -31,28 +31,35 @@ namespace ts.server.typingsInstaller {
3131
}
3232

3333
/*@internal*/
34-
export function installNpmPackages(npmPath: string, tsVersion: string, packageNames: string[], install: (command: string) => boolean) {
34+
export function installNpmPackages(npmPath: string, tsVersion: string, packageNames: string[], install: (file: string, args: string[]) => boolean) {
3535
let hasError = false;
3636
for (let remaining = packageNames.length; remaining > 0;) {
3737
const result = getNpmCommandForInstallation(npmPath, tsVersion, packageNames, remaining);
3838
remaining = result.remaining;
39-
hasError = install(result.command) || hasError;
39+
hasError = install(result.command[0], result.command[1]) || hasError;
4040
}
4141
return hasError;
4242
}
4343

44+
function getUserAgent(tsVersion: string) {
45+
return `--user-agent="typesInstaller/${tsVersion}"`;
46+
}
47+
const npmInstall = "install", ignoreScripts = "--ignore-scripts", saveDev = "--save-dev";
48+
const commandBaseLength = npmInstall.length + ignoreScripts.length + saveDev.length + getUserAgent("").length + 5;
4449
/*@internal*/
4550
export function getNpmCommandForInstallation(npmPath: string, tsVersion: string, packageNames: string[], remaining: number) {
4651
const sliceStart = packageNames.length - remaining;
47-
let command: string, toSlice = remaining;
52+
let packages: string[], toSlice = remaining;
4853
while (true) {
49-
command = `${npmPath} install --ignore-scripts ${(toSlice === packageNames.length ? packageNames : packageNames.slice(sliceStart, sliceStart + toSlice)).join(" ")} --save-dev --user-agent="typesInstaller/${tsVersion}"`;
50-
if (command.length < 8000) {
54+
packages = toSlice === packageNames.length ? packageNames : packageNames.slice(sliceStart, sliceStart + toSlice);
55+
const commandLength = npmPath.length + commandBaseLength + packages.join(" ").length + tsVersion.length;
56+
if (commandLength < 8000) {
5157
break;
5258
}
5359

5460
toSlice = toSlice - Math.floor(toSlice / 2);
5561
}
62+
const command: [string, string[]] = [npmPath, [npmInstall, ignoreScripts, ...packages, saveDev, getUserAgent(tsVersion)]];
5663
return { command, remaining: remaining - toSlice };
5764
}
5865

0 commit comments

Comments
 (0)