Respond to code review

Author: Andy Hanson

src/compiler/moduleNameResolver.ts

@@ -1048,18 +1048,27 @@ namespace ts {
     const mangledScopedPackageSeparator = "__";
 
     /** For a scoped package, we must look in `@types/foo__bar` instead of `@types/@foo/bar`. */
-    function mangleScopedPackage(moduleName: string, state: ModuleResolutionState): string {
-        if (startsWith(moduleName, "@")) {
-            const replaceSlash = moduleName.replace(ts.directorySeparator, mangledScopedPackageSeparator);
-            if (replaceSlash !== moduleName) {
-                const mangled = replaceSlash.slice(1); // Take off the "@"
-                if (state.traceEnabled) {
-                    trace(state.host, Diagnostics.Scoped_package_detected_looking_in_0, mangled);
-                }
-                return mangled;
+    function mangleScopedPackage(packageName: string, state: ModuleResolutionState): string {
+        const mangled = getMangledNameForScopedPackage(packageName);
+        if (state.traceEnabled && mangled !== packageName) {
+            trace(state.host, Diagnostics.Scoped_package_detected_looking_in_0, mangled);
+        }
+        return mangled;
+    }
+
+    /* @internal */
+    export function getTypesPackageName(packageName: string): string {
+        return `@types/${getMangledNameForScopedPackage(packageName)}`;
+    }
+
+    function getMangledNameForScopedPackage(packageName: string): string {
+        if (startsWith(packageName, "@")) {
+            const replaceSlash = packageName.replace(ts.directorySeparator, mangledScopedPackageSeparator);
+            if (replaceSlash !== packageName) {
+                return replaceSlash.slice(1); // Take off the "@"
             }
         }
-        return moduleName;
+        return packageName;
     }
 
     /* @internal */

src/harness/harnessLanguageService.ts

@@ -183,11 +183,11 @@ namespace Harness.LanguageService {
 
     /// Native adapter
     class NativeLanguageServiceHost extends LanguageServiceAdapterHost implements ts.LanguageServiceHost {
-        tryGetTypesRegistry(): ts.Map<void> | undefined {
+        isKnownTypesPackageName(name: string): boolean {
             if (this.typesRegistry === undefined) {
                 ts.Debug.fail("fourslash test should set types registry.");
             }
-            return this.typesRegistry;
+            return this.typesRegistry.has(name);
         }
         installPackage = ts.notImplemented;
 

src/harness/unittests/languageService.ts

@@ -21,8 +21,6 @@ export function Component(x: Config): any;`
         //  to write an alias to a module's default export was referrenced across files and had no default export
         it("should be able to create a language service which can respond to deinition requests without throwing", () => {
             const languageService = ts.createLanguageService({
-                tryGetTypesRegistry: notImplemented,
-                installPackage: notImplemented,
                 getCompilationSettings() {
                     return {};
                 },

src/harness/unittests/session.ts

@@ -372,7 +372,7 @@ namespace ts.server {
                 };
                 const command = "test";
 
-                session.output(body, command, /*reqSeq*/ 0, /*success*/ true);
+                session.output(body, command, /*reqSeq*/ 0);
 
                 expect(lastSent).to.deep.equal({
                     seq: 0,
@@ -475,7 +475,7 @@ namespace ts.server {
             };
             const command = "test";
 
-            session.output(body, command, /*reqSeq*/ 0, /*success*/ true);
+            session.output(body, command, /*reqSeq*/ 0);
 
             expect(session.lastSent).to.deep.equal({
                 seq: 0,
@@ -549,7 +549,7 @@ namespace ts.server {
                     return;
                 }
                 if (response) {
-                    this.output(response, msg.command, msg.seq, /*success*/ true);
+                    this.output(response, msg.command, msg.seq);
                 }
             }
 

src/harness/unittests/tsserverProjectSystem.ts

@@ -70,7 +70,7 @@ namespace ts.projectSystem {
 
         protected postExecActions: PostExecAction[] = [];
 
-        tryGetTypesRegistry = notImplemented;
+        isKnownTypesPackageName = notImplemented;
         installPackage = notImplemented;
 
         executePendingCommands() {

src/server/client.ts

@@ -540,15 +540,7 @@ namespace ts.server {
             return response.body.map(entry => this.convertCodeActions(entry, file));
         }
 
-        applyCodeActionCommand(file: string, command: CodeActionCommand): PromiseLike<ApplyCodeActionCommandResult> {
-            const args: protocol.ApplyCodeActionCommandRequestArgs = { file, command };
-
-            const request = this.processRequest<protocol.ApplyCodeActionCommandRequest>(CommandNames.ApplyCodeActionCommand, args);
-            // TODO: how can we possibly get it synchronously here? But is SessionClient test-only?
-            const response = this.processResponse<protocol.ApplyCodeActionCommandResponse>(request);
-
-            return PromiseImpl.resolved({ successMessage: response.message });
-        }
+        applyCodeActionCommand = notImplemented;
 
         private createFileLocationOrRangeRequestArgs(positionOrRange: number | TextRange, fileName: string): protocol.FileLocationOrRangeRequestArgs {
             return typeof positionOrRange === "number"

src/server/project.ts

@@ -245,8 +245,8 @@ namespace ts.server {
         /*@internal*/
         protected abstract getProjectRootPath(): Path | undefined;
 
-        tryGetTypesRegistry(): Map<void> | undefined {
-            return this.typingsCache.tryGetTypesRegistry();
+        isKnownTypesPackageName(name: string): boolean {
+            return this.typingsCache.isKnownTypesPackageName(name);
         }
         installPackage(options: InstallPackageOptions): PromiseLike<ApplyCodeActionCommandResult> {
             return this.typingsCache.installPackage({ ...options, projectRootPath: this.getProjectRootPath() });

src/server/protocol.ts

@@ -1561,6 +1561,7 @@ namespace ts.server.protocol {
         description: string;
         /** Text changes to apply to each file as part of the code action */
         changes: FileCodeEdits[];
+        /** A command is an opaque object that should be passed to `ApplyCodeActionCommandRequestArgs` without modification.  */
         commands?: {}[];
     }
 

src/server/server.ts

@@ -250,6 +250,8 @@ namespace ts.server {
         private activeRequestCount = 0;
         private requestQueue: QueuedOperation[] = [];
         private requestMap = createMap<QueuedOperation>(); // Maps operation ID to newest requestQueue entry with that ID
+        /** We will lazily request the types registry on the first call to `isKnownTypesPackageName` and store it in `typesRegistryCache`. */
+        private requestedRegistry: boolean;
         private typesRegistryCache: Map<void> | undefined;
 
         // This number is essentially arbitrary.  Processing more than one typings request
@@ -279,13 +281,20 @@ namespace ts.server {
             }
         }
 
-        tryGetTypesRegistry(): Map<void> | undefined {
-            if (this.typesRegistryCache) {
-                return this.typesRegistryCache;
+        isKnownTypesPackageName(name: string): boolean {
+            // We want to avoid looking this up in the registry as that is expensive. So first check that it's actually an NPM package.
+            const validationResult = JsTyping.validatePackageName(name);
+            if (validationResult !== JsTyping.PackageNameValidationResult.Ok) {
+                return undefined;
             }
 
+            if (this.requestedRegistry) {
+                return !!this.typesRegistryCache && this.typesRegistryCache.has(name);
+            }
+
+            this.requestedRegistry = true;
             this.send({ kind: "typesRegistry" });
-            return undefined;
+            return false;
         }
 
         installPackage(options: InstallPackageOptionsWithProjectRootPath): PromiseLike<ApplyCodeActionCommandResult> {

src/server/session.ts

@@ -411,7 +411,12 @@ namespace ts.server {
             this.send(ev);
         }
 
-        public output(info: {} | undefined, cmdName: string, reqSeq: number, success: boolean, message?: string) {
+        // For backwards-compatibility only.
+        public output(info: any, cmdName: string, reqSeq?: number, errorMsg?: string): void {
+            this.doOutput(info, cmdName, reqSeq, /*success*/ !errorMsg, errorMsg);
+        }
+
+        private doOutput(info: {} | undefined, cmdName: string, reqSeq: number, success: boolean, message?: string): void {
             const res: protocol.Response = {
                 seq: 0,
                 type: "response",
@@ -1299,7 +1304,7 @@ namespace ts.server {
             this.changeSeq++;
             // make sure no changes happen before this one is finished
             if (project.reloadScript(file, tempFileName)) {
-                this.output(undefined, CommandNames.Reload, reqSeq, /*success*/ true);
+                this.doOutput(/*info*/ undefined, CommandNames.Reload, reqSeq, /*success*/ true);
             }
         }
 
@@ -1539,7 +1544,7 @@ namespace ts.server {
 
         private applyCodeActionCommand(commandName: string, requestSeq: number, args: protocol.ApplyCodeActionCommandRequestArgs): void {
             const { file, project } = this.getFileAndProject(args);
-            const output = (success: boolean, message: string) => this.output({}, commandName, requestSeq, success, message);
+            const output = (success: boolean, message: string) => this.doOutput({}, commandName, requestSeq, success, message);
             const command = args.command as CodeActionCommand; // They should be sending back the command we sent them.
             project.getLanguageService().applyCodeActionCommand(file, command).then(
                 ({ successMessage }) => { output(/*success*/ true, successMessage); },
@@ -1845,7 +1850,7 @@ namespace ts.server {
             },
             [CommandNames.Configure]: (request: protocol.ConfigureRequest) => {
                 this.projectService.setHostConfiguration(request.arguments);
-                this.output(undefined, CommandNames.Configure, request.seq, /*success*/ true);
+                this.doOutput(/*info*/ undefined, CommandNames.Configure, request.seq, /*success*/ true);
                 return this.notRequired();
             },
             [CommandNames.Reload]: (request: protocol.ReloadRequest) => {
@@ -1966,7 +1971,7 @@ namespace ts.server {
             }
             else {
                 this.logger.msg(`Unrecognized JSON command: ${JSON.stringify(request)}`, Msg.Err);
-                this.output(undefined, CommandNames.Unknown, request.seq, /*success*/ false, `Unrecognized JSON command: ${request.command}`);
+                this.doOutput(/*info*/ undefined, CommandNames.Unknown, request.seq, /*success*/ false, `Unrecognized JSON command: ${request.command}`);
                 return { responseRequired: false };
             }
         }
@@ -1997,21 +2002,21 @@ namespace ts.server {
                 }
 
                 if (response) {
-                    this.output(response, request.command, request.seq, /*success*/ true);
+                    this.doOutput(response, request.command, request.seq, /*success*/ true);
                 }
                 else if (responseRequired) {
-                    this.output(undefined, request.command, request.seq, /*success*/ false, "No content available.");
+                    this.doOutput(/*info*/ undefined, request.command, request.seq, /*success*/ false, "No content available.");
                 }
             }
             catch (err) {
                 if (err instanceof OperationCanceledException) {
                     // Handle cancellation exceptions
-                    this.output({ canceled: true }, request.command, request.seq, /*success*/ true);
+                    this.doOutput({ canceled: true }, request.command, request.seq, /*success*/ true);
                     return;
                 }
                 this.logError(err, message);
-                this.output(
-                    undefined,
+                this.doOutput(
+                    /*info*/ undefined,
                     request ? request.command : CommandNames.Unknown,
                     request ? request.seq : 0,
                     /*success*/ false,

src/server/typingsCache.ts

@@ -6,7 +6,7 @@ namespace ts.server {
     }
 
     export interface ITypingsInstaller {
-        tryGetTypesRegistry(): Map<void> | undefined;
+        isKnownTypesPackageName(name: string): boolean;
         installPackage(options: InstallPackageOptionsWithProjectRootPath): PromiseLike<ApplyCodeActionCommandResult>;
         enqueueInstallTypingsRequest(p: Project, typeAcquisition: TypeAcquisition, unresolvedImports: SortedReadonlyArray<string>): void;
         attach(projectService: ProjectService): void;
@@ -15,7 +15,7 @@ namespace ts.server {
     }
 
     export const nullTypingsInstaller: ITypingsInstaller = {
-        tryGetTypesRegistry: () => undefined,
+        isKnownTypesPackageName: returnFalse,
         // Should never be called because we never provide a types registry.
         installPackage: notImplemented,
         enqueueInstallTypingsRequest: noop,
@@ -86,8 +86,8 @@ namespace ts.server {
         constructor(private readonly installer: ITypingsInstaller) {
         }
 
-        tryGetTypesRegistry(): Map<void> | undefined {
-            return this.installer.tryGetTypesRegistry();
+        isKnownTypesPackageName(name: string): boolean {
+            return this.installer.isKnownTypesPackageName(name);
         }
 
         installPackage(options: InstallPackageOptionsWithProjectRootPath): PromiseLike<ApplyCodeActionCommandResult> {

src/services/codefixes/fixCannotFindModule.ts

@@ -20,19 +20,12 @@ namespace ts.codefix {
     export function tryGetCodeActionForInstallPackageTypes(host: LanguageServiceHost, moduleName: string): CodeAction | undefined {
         const { packageName } = getPackageName(moduleName);
 
-        // We want to avoid looking this up in the registry as that is expensive. So first check that it's actually an NPM package.
-        const validationResult = JsTyping.validatePackageName(packageName);
-        if (validationResult !== JsTyping.PackageNameValidationResult.Ok) {
-            return undefined;
-        }
-
-        const registry = host.tryGetTypesRegistry();
-        if (!registry || !registry.has(packageName)) {
+        if (!host.isKnownTypesPackageName(moduleName)) {
             // If !registry, registry not available yet, can't do anything.
             return undefined;
         }
 
-        const typesPackageName = `@types/${packageName}`;
+        const typesPackageName = getTypesPackageName(packageName);
         return {
             description: `Install '${typesPackageName}'`,
             changes: [],

src/services/services.ts

@@ -1765,7 +1765,9 @@ namespace ts {
             fileName = toPath(fileName, currentDirectory, getCanonicalFileName);
             switch (action.type) {
                 case "install package":
-                    return host.installPackage({ fileName, packageName: action.packageName });
+                    return host.installPackage
+                        ? host.installPackage({ fileName, packageName: action.packageName })
+                        : PromiseImpl.reject("Host does not implement `installPackage`");
                 default:
                     Debug.fail();
                     // TODO: Debug.assertNever(action); will only work if there is more than one type.

src/services/shims.ts

@@ -347,16 +347,6 @@ namespace ts {
             }
         }
 
-        public tryGetTypesRegistry(): Map<void> | undefined {
-            throw new Error("TODO");
-        }
-        public installPackage(_options: InstallPackageOptions): PromiseLike<ApplyCodeActionCommandResult> {
-            throw new Error("TODO");
-        }
-        public getTsconfigLocation(): Path | undefined {
-            throw new Error("TODO");
-        }
-
         public log(s: string): void {
             if (this.loggingEnabled) {
                 this.shimHost.log(s);

src/services/types.ts

@@ -203,7 +203,7 @@ namespace ts {
          */
         getCustomTransformers?(): CustomTransformers | undefined;
 
-        tryGetTypesRegistry?(): Map<void> | undefined;
+        isKnownTypesPackageName?(name: string): boolean;
         installPackage?(options: InstallPackageOptions): PromiseLike<ApplyCodeActionCommandResult>;
     }
 

src/services/utilities.ts

@@ -1107,10 +1107,14 @@ namespace ts {
             return new PromiseImpl(PromiseState.Unresolved, undefined, undefined, undefined);
         }
 
-        static resolved<T>(value: T): PromiseImpl<T> {
+        static resolve<T>(value: T): PromiseImpl<T> {
             return new PromiseImpl<T>(PromiseState.Success, value, undefined, undefined);
         }
 
+        static reject(value: {}): PromiseImpl<never> {
+            return new PromiseImpl<never>(PromiseState.Failure, undefined as never, value, undefined);
+        }
+
         resolve(value: T): void {
             Debug.assert(this.state === PromiseState.Unresolved);
             this.state = PromiseState.Success;
@@ -1176,7 +1180,7 @@ namespace ts {
     }
 
     function toPromiseLike<T>(x: T | PromiseLike<T>): PromiseLike<T> {
-        return isPromiseLike(x) ? x as PromiseLike<T> : PromiseImpl.resolved(x as T);
+        return isPromiseLike(x) ? x as PromiseLike<T> : PromiseImpl.resolve(x as T);
     }
 }