Skip to content

Use execFileSync in typing installer #29816

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 15, 2019
Merged

Use execFileSync in typing installer #29816

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions src/testRunner/unittests/tsserver/typingsInstaller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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[] = [];
const hasError = TI.installNpmPackages(npmPath, tsVersion, packageNames, command => {
commands.push(command);
const commands: [string, string[]][] = [];
const hasError = TI.installNpmPackages(npmPath, tsVersion, packageNames, (file, args) => {
commands.push([file, args]);
return false;
});
assert.isFalse(hasError);
assert.deepEqual(commands, expectedCommands, "commands");
});

it("installs remaining packages when one of the partial command fails", () => {
const commands: string[] = [];
const hasError = TI.installNpmPackages(npmPath, tsVersion, packageNames, command => {
commands.push(command);
const commands: [string, string[]][] = [];
const hasError = TI.installNpmPackages(npmPath, tsVersion, packageNames, (file, args) => {
commands.push([file, args]);
return commands.length === 1;
});
assert.isTrue(hasError);
Expand Down
16 changes: 8 additions & 8 deletions src/typingsInstaller/nodeTypingsInstaller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,10 +70,10 @@ namespace ts.server.typingsInstaller {
cwd: string;
encoding: "utf-8";
}
type ExecSync = (command: string, options: ExecSyncOptions) => string;
type ExecFileSync = (file: string, args: string[], options: ExecSyncOptions) => string;

export class NodeTypingsInstaller extends TypingsInstaller {
private readonly nodeExecSync: ExecSync;
private readonly nodeExecFileSync: ExecFileSync;
private readonly npmPath: string;
readonly typesRegistry: Map<MapLike<string>>;

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

this.ensurePackageDirectoryExists(globalTypingsCacheLocation);

try {
if (this.log.isEnabled()) {
this.log.writeLine(`Updating ${typesRegistryPackageName} npm package...`);
}
this.execSyncAndLog(`${this.npmPath} install --ignore-scripts ${typesRegistryPackageName}@${this.latestDistTag}`, { cwd: globalTypingsCacheLocation });
this.execFileSyncAndLog(this.npmPath, ["install", "--ignore-scripts", `${typesRegistryPackageName}@${this.latestDistTag}`], { cwd: globalTypingsCacheLocation });
if (this.log.isEnabled()) {
this.log.writeLine(`Updated ${typesRegistryPackageName} npm package`);
}
Expand Down Expand Up @@ -189,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, command => this.execSyncAndLog(command, { cwd }));
const hasError = installNpmPackages(this.npmPath, version, packageNames, (file, args) => this.execFileSyncAndLog(file, args, { 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 execSyncAndLog(command: string, options: Pick<ExecSyncOptions, "cwd">): boolean {
private execFileSyncAndLog(file: string, args: string[], options: Pick<ExecSyncOptions, "cwd">): boolean {
if (this.log.isEnabled()) {
this.log.writeLine(`Exec: ${command}`);
this.log.writeLine(`Exec: ${file} ${args.join(" ")}`);
}
try {
const stdout = this.nodeExecSync(command, { ...options, encoding: "utf-8" });
const stdout = this.nodeExecFileSync(file, args, { ...options, encoding: "utf-8" });
if (this.log.isEnabled()) {
this.log.writeLine(` Succeeded. stdout:${indent(sys.newLine, stdout)}`);
}
Expand Down
17 changes: 12 additions & 5 deletions src/typingsInstallerCore/typingsInstaller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,28 +31,35 @@ namespace ts.server.typingsInstaller {
}

/*@internal*/
export function installNpmPackages(npmPath: string, tsVersion: string, packageNames: string[], install: (command: string) => boolean) {
export function installNpmPackages(npmPath: string, tsVersion: string, packageNames: string[], install: (file: string, args: 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) || hasError;
hasError = install(result.command[0], result.command[1]) || 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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should just say getUserAgent("0.0.0") rather than adding the + 5, IMO.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 5 is for the spaces around them (the tsversion length is added later on while computing actual command length as we use to.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah. That was not immediately clear, sorry.

/*@internal*/
export function getNpmCommandForInstallation(npmPath: string, tsVersion: string, packageNames: string[], remaining: number) {
const sliceStart = packageNames.length - remaining;
let command: string, toSlice = remaining;
let packages: string[], toSlice = remaining;
while (true) {
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) {
packages = toSlice === packageNames.length ? packageNames : packageNames.slice(sliceStart, sliceStart + toSlice);
const commandLength = npmPath.length + commandBaseLength + packages.join(" ").length + tsVersion.length;
if (commandLength < 8000) {
break;
}

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

Expand Down