Merge pull request #30110 from Microsoft/revertExecFileSync

Revert execFileSync
microsoft · Feb 27, 2019 · 4718ff8 · 4718ff8
2 parents e8e7e88 + fd10c12
commit 4718ff8
Show file tree

Hide file tree

Showing 3 changed files with 23 additions and 26 deletions.
diff --git a/src/testRunner/unittests/tsserver/typingsInstaller.ts b/src/testRunner/unittests/tsserver/typingsInstaller.ts
@@ -1684,19 +1684,19 @@ namespace ts.projectSystem {
             TI.getNpmCommandForInstallation(npmPath, tsVersion, packageNames, packageNames.length - Math.ceil(packageNames.length / 2)).command
         ];
         it("works when the command is too long to install all packages at once", () => {
-            const commands: [string, string[]][] = [];
-            const hasError = TI.installNpmPackages(npmPath, tsVersion, packageNames, (file, args) => {
-                commands.push([file, args]);
+            const commands: string[] = [];
+            const hasError = TI.installNpmPackages(npmPath, tsVersion, packageNames, command => {
+                commands.push(command);
                 return false;
             });
             assert.isFalse(hasError);
             assert.deepEqual(commands, expectedCommands, "commands");
         });
 
         it("installs remaining packages when one of the partial command fails", () => {
-            const commands: [string, string[]][] = [];
-            const hasError = TI.installNpmPackages(npmPath, tsVersion, packageNames, (file, args) => {
-                commands.push([file, args]);
+            const commands: string[] = [];
+            const hasError = TI.installNpmPackages(npmPath, tsVersion, packageNames, command => {
+                commands.push(command);
                 return commands.length === 1;
             });
             assert.isTrue(hasError);

diff --git a/src/typingsInstaller/nodeTypingsInstaller.ts b/src/typingsInstaller/nodeTypingsInstaller.ts
@@ -70,10 +70,10 @@ namespace ts.server.typingsInstaller {
         cwd: string;
         encoding: "utf-8";
     }
-    type ExecFileSync = (file: string, args: string[], options: ExecSyncOptions) => string;
+    type ExecSync = (command: string, options: ExecSyncOptions) => string;
 
     export class NodeTypingsInstaller extends TypingsInstaller {
-        private readonly nodeExecFileSync: ExecFileSync;
+        private readonly nodeExecSync: ExecSync;
         private readonly npmPath: string;
         readonly typesRegistry: Map<MapLike<string>>;
 
@@ -89,19 +89,23 @@ namespace ts.server.typingsInstaller {
                 log);
             this.npmPath = npmLocation !== undefined ? npmLocation : getDefaultNPMLocation(process.argv[0]);
 
+            // If the NPM path contains spaces and isn't wrapped in quotes, do so.
+            if (stringContains(this.npmPath, " ") && this.npmPath[0] !== `"`) {
+                this.npmPath = `"${this.npmPath}"`;
+            }
             if (this.log.isEnabled()) {
                 this.log.writeLine(`Process id: ${process.pid}`);
                 this.log.writeLine(`NPM location: ${this.npmPath} (explicit '${Arguments.NpmLocation}' ${npmLocation === undefined ? "not " : ""} provided)`);
             }
-            ({ execFileSync: this.nodeExecFileSync } = require("child_process"));
+            ({ execSync: this.nodeExecSync } = require("child_process"));
 
             this.ensurePackageDirectoryExists(globalTypingsCacheLocation);
 
             try {
                 if (this.log.isEnabled()) {
                     this.log.writeLine(`Updating ${typesRegistryPackageName} npm package...`);
                 }
-                this.execFileSyncAndLog(this.npmPath, ["install", "--ignore-scripts", `${typesRegistryPackageName}@${this.latestDistTag}`], { cwd: globalTypingsCacheLocation });
+                this.execSyncAndLog(`${this.npmPath} install --ignore-scripts ${typesRegistryPackageName}@${this.latestDistTag}`, { cwd: globalTypingsCacheLocation });
                 if (this.log.isEnabled()) {
                     this.log.writeLine(`Updated ${typesRegistryPackageName} npm package`);
                 }
@@ -185,20 +189,20 @@ namespace ts.server.typingsInstaller {
                 this.log.writeLine(`#${requestId} with arguments'${JSON.stringify(packageNames)}'.`);
             }
             const start = Date.now();
-            const hasError = installNpmPackages(this.npmPath, version, packageNames, (file, args) => this.execFileSyncAndLog(file, args, { cwd }));
+            const hasError = installNpmPackages(this.npmPath, version, packageNames, command => this.execSyncAndLog(command, { cwd }));
             if (this.log.isEnabled()) {
                 this.log.writeLine(`npm install #${requestId} took: ${Date.now() - start} ms`);
             }
             onRequestCompleted(!hasError);
         }
 
         /** Returns 'true' in case of error. */
-        private execFileSyncAndLog(file: string, args: string[], options: Pick<ExecSyncOptions, "cwd">): boolean {
+        private execSyncAndLog(command: string, options: Pick<ExecSyncOptions, "cwd">): boolean {
             if (this.log.isEnabled()) {
-                this.log.writeLine(`Exec: ${file} ${args.join(" ")}`);
+                this.log.writeLine(`Exec: ${command}`);
             }
             try {
-                const stdout = this.nodeExecFileSync(file, args, { ...options, encoding: "utf-8" });
+                const stdout = this.nodeExecSync(command, { ...options, encoding: "utf-8" });
                 if (this.log.isEnabled()) {
                     this.log.writeLine(`    Succeeded. stdout:${indent(sys.newLine, stdout)}`);
                 }

diff --git a/src/typingsInstallerCore/typingsInstaller.ts b/src/typingsInstallerCore/typingsInstaller.ts
@@ -31,35 +31,28 @@ namespace ts.server.typingsInstaller {
     }
 
     /*@internal*/
-    export function installNpmPackages(npmPath: string, tsVersion: string, packageNames: string[], install: (file: string, args: string[]) => boolean) {
+    export function installNpmPackages(npmPath: string, tsVersion: string, packageNames: string[], install: (command: string) => boolean) {
         let hasError = false;
         for (let remaining = packageNames.length; remaining > 0;) {
             const result = getNpmCommandForInstallation(npmPath, tsVersion, packageNames, remaining);
             remaining = result.remaining;
-            hasError = install(result.command[0], result.command[1]) || hasError;
+            hasError = install(result.command) || hasError;
         }
         return hasError;
     }
 
-    function getUserAgent(tsVersion: string) {
-        return `--user-agent="typesInstaller/${tsVersion}"`;
-    }
-    const npmInstall = "install", ignoreScripts = "--ignore-scripts", saveDev = "--save-dev";
-    const commandBaseLength = npmInstall.length + ignoreScripts.length + saveDev.length + getUserAgent("").length + 5;
     /*@internal*/
     export function getNpmCommandForInstallation(npmPath: string, tsVersion: string, packageNames: string[], remaining: number) {
         const sliceStart = packageNames.length - remaining;
-        let packages: string[], toSlice = remaining;
+        let command: string, toSlice = remaining;
         while (true) {
-            packages = toSlice === packageNames.length ? packageNames : packageNames.slice(sliceStart, sliceStart + toSlice);
-            const commandLength = npmPath.length + commandBaseLength + packages.join(" ").length + tsVersion.length;
-            if (commandLength < 8000) {
+            command = `${npmPath} install --ignore-scripts ${(toSlice === packageNames.length ? packageNames : packageNames.slice(sliceStart, sliceStart + toSlice)).join(" ")} --save-dev --user-agent="typesInstaller/${tsVersion}"`;
+            if (command.length < 8000) {
                 break;
             }
 
             toSlice = toSlice - Math.floor(toSlice / 2);
         }
-        const command: [string, string[]] = [npmPath, [npmInstall, ignoreScripts, ...packages, saveDev, getUserAgent(tsVersion)]];
         return { command, remaining: remaining - toSlice };
     }