Skip to content

Commit f7a2288

Browse files
author
Andy Hanson
committed
Respond to code review
1 parent ee9f892 commit f7a2288

15 files changed

+70
-67
lines changed

Diff for: ‎src/compiler/moduleNameResolver.ts

+19-10
Original file line numberDiff line numberDiff line change
@@ -1048,18 +1048,27 @@ namespace ts {
10481048
const mangledScopedPackageSeparator = "__";
10491049

10501050
/** For a scoped package, we must look in `@types/foo__bar` instead of `@types/@foo/bar`. */
1051-
function mangleScopedPackage(moduleName: string, state: ModuleResolutionState): string {
1052-
if (startsWith(moduleName, "@")) {
1053-
const replaceSlash = moduleName.replace(ts.directorySeparator, mangledScopedPackageSeparator);
1054-
if (replaceSlash !== moduleName) {
1055-
const mangled = replaceSlash.slice(1); // Take off the "@"
1056-
if (state.traceEnabled) {
1057-
trace(state.host, Diagnostics.Scoped_package_detected_looking_in_0, mangled);
1058-
}
1059-
return mangled;
1051+
function mangleScopedPackage(packageName: string, state: ModuleResolutionState): string {
1052+
const mangled = getMangledNameForScopedPackage(packageName);
1053+
if (state.traceEnabled && mangled !== packageName) {
1054+
trace(state.host, Diagnostics.Scoped_package_detected_looking_in_0, mangled);
1055+
}
1056+
return mangled;
1057+
}
1058+
1059+
/* @internal */
1060+
export function getTypesPackageName(packageName: string): string {
1061+
return `@types/${getMangledNameForScopedPackage(packageName)}`;
1062+
}
1063+
1064+
function getMangledNameForScopedPackage(packageName: string): string {
1065+
if (startsWith(packageName, "@")) {
1066+
const replaceSlash = packageName.replace(ts.directorySeparator, mangledScopedPackageSeparator);
1067+
if (replaceSlash !== packageName) {
1068+
return replaceSlash.slice(1); // Take off the "@"
10601069
}
10611070
}
1062-
return moduleName;
1071+
return packageName;
10631072
}
10641073

10651074
/* @internal */

Diff for: ‎src/harness/harnessLanguageService.ts

+2-2
Original file line numberDiff line numberDiff line change
@@ -183,11 +183,11 @@ namespace Harness.LanguageService {
183183

184184
/// Native adapter
185185
class NativeLanguageServiceHost extends LanguageServiceAdapterHost implements ts.LanguageServiceHost {
186-
tryGetTypesRegistry(): ts.Map<void> | undefined {
186+
isKnownTypesPackageName(name: string): boolean {
187187
if (this.typesRegistry === undefined) {
188188
ts.Debug.fail("fourslash test should set types registry.");
189189
}
190-
return this.typesRegistry;
190+
return this.typesRegistry.has(name);
191191
}
192192
installPackage = ts.notImplemented;
193193

Diff for: ‎src/harness/unittests/languageService.ts

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

Diff for: ‎src/harness/unittests/tsserverProjectSystem.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@ namespace ts.projectSystem {
7070

7171
protected postExecActions: PostExecAction[] = [];
7272

73-
tryGetTypesRegistry = notImplemented;
73+
isKnownTypesPackageName = notImplemented;
7474
installPackage = notImplemented;
7575

7676
executePendingCommands() {

Diff for: ‎src/server/client.ts

+1-9
Original file line numberDiff line numberDiff line change
@@ -540,15 +540,7 @@ namespace ts.server {
540540
return response.body.map(entry => this.convertCodeActions(entry, file));
541541
}
542542

543-
applyCodeActionCommand(file: string, command: CodeActionCommand): PromiseLike<ApplyCodeActionCommandResult> {
544-
const args: protocol.ApplyCodeActionCommandRequestArgs = { file, command };
545-
546-
const request = this.processRequest<protocol.ApplyCodeActionCommandRequest>(CommandNames.ApplyCodeActionCommand, args);
547-
// TODO: how can we possibly get it synchronously here? But is SessionClient test-only?
548-
const response = this.processResponse<protocol.ApplyCodeActionCommandResponse>(request);
549-
550-
return PromiseImpl.resolved({ successMessage: response.message });
551-
}
543+
applyCodeActionCommand = notImplemented;
552544

553545
private createFileLocationOrRangeRequestArgs(positionOrRange: number | TextRange, fileName: string): protocol.FileLocationOrRangeRequestArgs {
554546
return typeof positionOrRange === "number"

Diff for: ‎src/server/project.ts

+2-2
Original file line numberDiff line numberDiff line change
@@ -245,8 +245,8 @@ namespace ts.server {
245245
/*@internal*/
246246
protected abstract getProjectRootPath(): Path | undefined;
247247

248-
tryGetTypesRegistry(): Map<void> | undefined {
249-
return this.typingsCache.tryGetTypesRegistry();
248+
isKnownTypesPackageName(name: string): boolean {
249+
return this.typingsCache.isKnownTypesPackageName(name);
250250
}
251251
installPackage(options: InstallPackageOptions): PromiseLike<ApplyCodeActionCommandResult> {
252252
return this.typingsCache.installPackage({ ...options, projectRootPath: this.getProjectRootPath() });

Diff for: ‎src/server/protocol.ts

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

Diff for: ‎src/server/server.ts

+13-4
Original file line numberDiff line numberDiff line change
@@ -250,6 +250,8 @@ namespace ts.server {
250250
private activeRequestCount = 0;
251251
private requestQueue: QueuedOperation[] = [];
252252
private requestMap = createMap<QueuedOperation>(); // Maps operation ID to newest requestQueue entry with that ID
253+
/** We will lazily request the types registry on the first call to `isKnownTypesPackageName` and store it in `typesRegistryCache`. */
254+
private requestedRegistry: boolean;
253255
private typesRegistryCache: Map<void> | undefined;
254256

255257
// This number is essentially arbitrary. Processing more than one typings request
@@ -279,13 +281,20 @@ namespace ts.server {
279281
}
280282
}
281283

282-
tryGetTypesRegistry(): Map<void> | undefined {
283-
if (this.typesRegistryCache) {
284-
return this.typesRegistryCache;
284+
isKnownTypesPackageName(name: string): boolean {
285+
// We want to avoid looking this up in the registry as that is expensive. So first check that it's actually an NPM package.
286+
const validationResult = JsTyping.validatePackageName(name);
287+
if (validationResult !== JsTyping.PackageNameValidationResult.Ok) {
288+
return undefined;
285289
}
286290

291+
if (this.requestedRegistry) {
292+
return !!this.typesRegistryCache && this.typesRegistryCache.has(name);
293+
}
294+
295+
this.requestedRegistry = true;
287296
this.send({ kind: "typesRegistry" });
288-
return undefined;
297+
return false;
289298
}
290299

291300
installPackage(options: InstallPackageOptionsWithProjectRootPath): PromiseLike<ApplyCodeActionCommandResult> {

Diff for: ‎src/server/session.ts

+15-10
Original file line numberDiff line numberDiff line change
@@ -411,7 +411,12 @@ namespace ts.server {
411411
this.send(ev);
412412
}
413413

414-
public output(info: {} | undefined, cmdName: string, reqSeq: number, success: boolean, message?: string) {
414+
// For backwards-compatibility only.
415+
public output(info: any, cmdName: string, reqSeq?: number, errorMsg?: string): void {
416+
this.doOutput(info, cmdName, reqSeq, /*success*/ !errorMsg, errorMsg);
417+
}
418+
419+
private doOutput(info: {} | undefined, cmdName: string, reqSeq: number, success: boolean, message?: string): void {
415420
const res: protocol.Response = {
416421
seq: 0,
417422
type: "response",
@@ -1299,7 +1304,7 @@ namespace ts.server {
12991304
this.changeSeq++;
13001305
// make sure no changes happen before this one is finished
13011306
if (project.reloadScript(file, tempFileName)) {
1302-
this.output(undefined, CommandNames.Reload, reqSeq, /*success*/ true);
1307+
this.doOutput(/*info*/ undefined, CommandNames.Reload, reqSeq, /*success*/ true);
13031308
}
13041309
}
13051310

@@ -1539,7 +1544,7 @@ namespace ts.server {
15391544

15401545
private applyCodeActionCommand(commandName: string, requestSeq: number, args: protocol.ApplyCodeActionCommandRequestArgs): void {
15411546
const { file, project } = this.getFileAndProject(args);
1542-
const output = (success: boolean, message: string) => this.output({}, commandName, requestSeq, success, message);
1547+
const output = (success: boolean, message: string) => this.doOutput({}, commandName, requestSeq, success, message);
15431548
const command = args.command as CodeActionCommand; // They should be sending back the command we sent them.
15441549
project.getLanguageService().applyCodeActionCommand(file, command).then(
15451550
({ successMessage }) => { output(/*success*/ true, successMessage); },
@@ -1845,7 +1850,7 @@ namespace ts.server {
18451850
},
18461851
[CommandNames.Configure]: (request: protocol.ConfigureRequest) => {
18471852
this.projectService.setHostConfiguration(request.arguments);
1848-
this.output(undefined, CommandNames.Configure, request.seq, /*success*/ true);
1853+
this.doOutput(/*info*/ undefined, CommandNames.Configure, request.seq, /*success*/ true);
18491854
return this.notRequired();
18501855
},
18511856
[CommandNames.Reload]: (request: protocol.ReloadRequest) => {
@@ -1966,7 +1971,7 @@ namespace ts.server {
19661971
}
19671972
else {
19681973
this.logger.msg(`Unrecognized JSON command: ${JSON.stringify(request)}`, Msg.Err);
1969-
this.output(undefined, CommandNames.Unknown, request.seq, /*success*/ false, `Unrecognized JSON command: ${request.command}`);
1974+
this.doOutput(/*info*/ undefined, CommandNames.Unknown, request.seq, /*success*/ false, `Unrecognized JSON command: ${request.command}`);
19701975
return { responseRequired: false };
19711976
}
19721977
}
@@ -1997,21 +2002,21 @@ namespace ts.server {
19972002
}
19982003

19992004
if (response) {
2000-
this.output(response, request.command, request.seq, /*success*/ true);
2005+
this.doOutput(response, request.command, request.seq, /*success*/ true);
20012006
}
20022007
else if (responseRequired) {
2003-
this.output(undefined, request.command, request.seq, /*success*/ false, "No content available.");
2008+
this.doOutput(/*info*/ undefined, request.command, request.seq, /*success*/ false, "No content available.");
20042009
}
20052010
}
20062011
catch (err) {
20072012
if (err instanceof OperationCanceledException) {
20082013
// Handle cancellation exceptions
2009-
this.output({ canceled: true }, request.command, request.seq, /*success*/ true);
2014+
this.doOutput({ canceled: true }, request.command, request.seq, /*success*/ true);
20102015
return;
20112016
}
20122017
this.logError(err, message);
2013-
this.output(
2014-
undefined,
2018+
this.doOutput(
2019+
/*info*/ undefined,
20152020
request ? request.command : CommandNames.Unknown,
20162021
request ? request.seq : 0,
20172022
/*success*/ false,

Diff for: ‎src/server/typingsCache.ts

+4-4
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ namespace ts.server {
66
}
77

88
export interface ITypingsInstaller {
9-
tryGetTypesRegistry(): Map<void> | undefined;
9+
isKnownTypesPackageName(name: string): boolean;
1010
installPackage(options: InstallPackageOptionsWithProjectRootPath): PromiseLike<ApplyCodeActionCommandResult>;
1111
enqueueInstallTypingsRequest(p: Project, typeAcquisition: TypeAcquisition, unresolvedImports: SortedReadonlyArray<string>): void;
1212
attach(projectService: ProjectService): void;
@@ -15,7 +15,7 @@ namespace ts.server {
1515
}
1616

1717
export const nullTypingsInstaller: ITypingsInstaller = {
18-
tryGetTypesRegistry: () => undefined,
18+
isKnownTypesPackageName: returnFalse,
1919
// Should never be called because we never provide a types registry.
2020
installPackage: notImplemented,
2121
enqueueInstallTypingsRequest: noop,
@@ -86,8 +86,8 @@ namespace ts.server {
8686
constructor(private readonly installer: ITypingsInstaller) {
8787
}
8888

89-
tryGetTypesRegistry(): Map<void> | undefined {
90-
return this.installer.tryGetTypesRegistry();
89+
isKnownTypesPackageName(name: string): boolean {
90+
return this.installer.isKnownTypesPackageName(name);
9191
}
9292

9393
installPackage(options: InstallPackageOptionsWithProjectRootPath): PromiseLike<ApplyCodeActionCommandResult> {

Diff for: ‎src/services/codefixes/fixCannotFindModule.ts

+2-9
Original file line numberDiff line numberDiff line change
@@ -20,19 +20,12 @@ namespace ts.codefix {
2020
export function tryGetCodeActionForInstallPackageTypes(host: LanguageServiceHost, moduleName: string): CodeAction | undefined {
2121
const { packageName } = getPackageName(moduleName);
2222

23-
// We want to avoid looking this up in the registry as that is expensive. So first check that it's actually an NPM package.
24-
const validationResult = JsTyping.validatePackageName(packageName);
25-
if (validationResult !== JsTyping.PackageNameValidationResult.Ok) {
26-
return undefined;
27-
}
28-
29-
const registry = host.tryGetTypesRegistry();
30-
if (!registry || !registry.has(packageName)) {
23+
if (!host.isKnownTypesPackageName(moduleName)) {
3124
// If !registry, registry not available yet, can't do anything.
3225
return undefined;
3326
}
3427

35-
const typesPackageName = `@types/${packageName}`;
28+
const typesPackageName = getTypesPackageName(packageName);
3629
return {
3730
description: `Install '${typesPackageName}'`,
3831
changes: [],

Diff for: ‎src/services/services.ts

+3-1
Original file line numberDiff line numberDiff line change
@@ -1765,7 +1765,9 @@ namespace ts {
17651765
fileName = toPath(fileName, currentDirectory, getCanonicalFileName);
17661766
switch (action.type) {
17671767
case "install package":
1768-
return host.installPackage({ fileName, packageName: action.packageName });
1768+
return host.installPackage
1769+
? host.installPackage({ fileName, packageName: action.packageName })
1770+
: PromiseImpl.reject("Host does not implement `installPackage`");
17691771
default:
17701772
Debug.fail();
17711773
// TODO: Debug.assertNever(action); will only work if there is more than one type.

Diff for: ‎src/services/shims.ts

-10
Original file line numberDiff line numberDiff line change
@@ -347,16 +347,6 @@ namespace ts {
347347
}
348348
}
349349

350-
public tryGetTypesRegistry(): Map<void> | undefined {
351-
throw new Error("TODO");
352-
}
353-
public installPackage(_options: InstallPackageOptions): PromiseLike<ApplyCodeActionCommandResult> {
354-
throw new Error("TODO");
355-
}
356-
public getTsconfigLocation(): Path | undefined {
357-
throw new Error("TODO");
358-
}
359-
360350
public log(s: string): void {
361351
if (this.loggingEnabled) {
362352
this.shimHost.log(s);

Diff for: ‎src/services/types.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -203,7 +203,7 @@ namespace ts {
203203
*/
204204
getCustomTransformers?(): CustomTransformers | undefined;
205205

206-
tryGetTypesRegistry?(): Map<void> | undefined;
206+
isKnownTypesPackageName?(name: string): boolean;
207207
installPackage?(options: InstallPackageOptions): PromiseLike<ApplyCodeActionCommandResult>;
208208
}
209209

Diff for: ‎src/services/utilities.ts

+6-2
Original file line numberDiff line numberDiff line change
@@ -1107,10 +1107,14 @@ namespace ts {
11071107
return new PromiseImpl(PromiseState.Unresolved, undefined, undefined, undefined);
11081108
}
11091109

1110-
static resolved<T>(value: T): PromiseImpl<T> {
1110+
static resolve<T>(value: T): PromiseImpl<T> {
11111111
return new PromiseImpl<T>(PromiseState.Success, value, undefined, undefined);
11121112
}
11131113

1114+
static reject(value: {}): PromiseImpl<never> {
1115+
return new PromiseImpl<never>(PromiseState.Failure, undefined as never, value, undefined);
1116+
}
1117+
11141118
resolve(value: T): void {
11151119
Debug.assert(this.state === PromiseState.Unresolved);
11161120
this.state = PromiseState.Success;
@@ -1176,7 +1180,7 @@ namespace ts {
11761180
}
11771181

11781182
function toPromiseLike<T>(x: T | PromiseLike<T>): PromiseLike<T> {
1179-
return isPromiseLike(x) ? x as PromiseLike<T> : PromiseImpl.resolved(x as T);
1183+
return isPromiseLike(x) ? x as PromiseLike<T> : PromiseImpl.resolve(x as T);
11801184
}
11811185
}
11821186

0 commit comments

Comments
 (0)