Skip to content

add 'installSuccess' flag to telemetry, cache misses if npm install fails #12163

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
mhegazy merged 2 commits into from
Nov 11, 2016
Merged

add 'installSuccess' flag to telemetry, cache misses if npm install fails #12163

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b8034fa
add 'installSuccess' flag to telemetry, cache misses if npm install f…
vladima Nov 11, 2016
eac7c57
fix typo
vladima Nov 11, 2016
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
4 changes: 4 additions & 0 deletions src/server/protocol.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1964,6 +1964,10 @@ namespace ts.server.protocol {
* Comma separated list of installed typing packages
*/
installedPackages: string;
/**
* true if install request succeeded, otherwise - false
*/
installSuccess: boolean;
}

export interface NavBarResponse extends Response {
Expand Down
3 changes: 2 additions & 1 deletion src/server/server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,7 +265,8 @@ namespace ts.server {
const body: protocol.TypingsInstalledTelemetryEventBody = {
telemetryEventName: "typingsInstalled",
payload: {
installedPackages: response.packagesToInstall.join(",")
installedPackages: response.packagesToInstall.join(","),
installSuccess: response.installSuccess
}
};
const eventName: protocol.TelemetryEventName = "telemetry";
Expand Down
1 change: 1 addition & 0 deletions src/server/types.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ declare namespace ts.server {
export interface TypingsInstallEvent extends TypingInstallerResponse {
readonly packagesToInstall: ReadonlyArray<string>;
readonly kind: EventInstall;
readonly installSuccess: boolean;
}

export interface InstallTypingHost extends JsTyping.TypingResolutionHost {
Expand Down
4 changes: 2 additions & 2 deletions src/server/typingsInstaller/nodeTypingsInstaller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,8 +139,8 @@ namespace ts.server.typingsInstaller {
if (this.log.isEnabled()) {
this.log.writeLine(`npm install #${requestId} took: ${Date.now() - start} ms${sys.newLine}stdout: ${stdout}${sys.newLine}stderr: ${stderr}`);
}
// treat any output on stdout as success
onRequestCompleted(!!stdout);
// treat absence of error as success
onRequestCompleted(!err);
Copy link
Copy Markdown
Member

@billti billti Nov 11, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I assume we've confirmed that "npm install" returns a non-0 error code across platforms/versions if a package fails to install (and always returns 0 on success, not like "1" if it succeeded with extra info).

Copy link
Copy Markdown
Contributor Author

@vladima vladima Nov 11, 2016

Choose a reason for hiding this comment

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

yes, that is correct

});
}
}
Expand Down
16 changes: 12 additions & 4 deletions src/server/typingsInstaller/typingsInstaller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@ namespace ts.server.typingsInstaller {
this.knownCachesSet[cacheLocation] = true;
}

private filterAndMapToScopedName(typingsToInstall: string[]) {
private filterTypings(typingsToInstall: string[]) {
if (typingsToInstall.length === 0) {
return typingsToInstall;
}
Expand All @@ -227,7 +227,7 @@ namespace ts.server.typingsInstaller {
const validationResult = validatePackageName(typing);
if (validationResult === PackageNameValidationResult.Ok) {
if (typing in this.typesRegistry) {
result.push(`@types/${typing}`);
result.push(typing);
}
else {
if (this.log.isEnabled()) {
Expand Down Expand Up @@ -280,7 +280,8 @@ namespace ts.server.typingsInstaller {
if (this.log.isEnabled()) {
this.log.writeLine(`Installing typings ${JSON.stringify(typingsToInstall)}`);
}
const scopedTypings = this.filterAndMapToScopedName(typingsToInstall);
const filteredTypings = this.filterTypings(typingsToInstall);
const scopedTypings = filteredTypings.map(x => `@types/${x}`);
if (scopedTypings.length === 0) {
if (this.log.isEnabled()) {
this.log.writeLine(`All typings are known to be missing or invalid - no need to go any further`);
Expand All @@ -297,11 +298,18 @@ namespace ts.server.typingsInstaller {
if (this.telemetryEnabled) {
this.sendResponse(<TypingsInstallEvent>{
kind: EventInstall,
packagesToInstall: scopedTypings
packagesToInstall: scopedTypings,
installSuccess: ok
});
}

if (!ok) {
if (this.log.isEnabled()) {
this.log.writeLine(`install request failed, marking packages as missing to prevent repeated requests: ${JSON.stringify(filteredTypings)}`);
}
for (const typing of filteredTypings) {
this.missingTypingsSet[typing] = true;
}
Copy link
Copy Markdown
Member

@billti billti Nov 11, 2016

Choose a reason for hiding this comment

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

It's interesting that we mark all the packages as missing. I assume that the npm install command could fail because one package failed, but the rest installed correctly? Or does NPM install in an "all-or-nothing" fashion?

Copy link
Copy Markdown
Contributor Author

@vladima vladima Nov 11, 2016

Choose a reason for hiding this comment

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

it is all or nothing

return;
}

Expand Down