Merge pull request microsoft#32425 from microsoft/scopedPackageAquisition

sheetalkamat · web-flow · commit b589020bb36f · 2019-07-16T16:48:35.000-07:00
Handle scoped package names in typing installer
diff --git a/src/jsTyping/jsTyping.ts b/src/jsTyping/jsTyping.ts
@@ -289,9 +289,8 @@ namespace ts.JsTyping {
 
     }
 
-    export const enum PackageNameValidationResult {
+    export const enum NameValidationResult {
         Ok,
-        ScopedPackagesNotSupported,
         EmptyName,
         NameTooLong,
         NameStartsWithDot,
@@ -301,49 +300,77 @@ namespace ts.JsTyping {
 
     const maxPackageNameLength = 214;
 
+    export interface ScopedPackageNameValidationResult {
+        name: string;
+        isScopeName: boolean;
+        result: NameValidationResult;
+    }
+    export type PackageNameValidationResult = NameValidationResult | ScopedPackageNameValidationResult;
+
     /**
      * Validates package name using rules defined at https://docs.npmjs.com/files/package.json
      */
     export function validatePackageName(packageName: string): PackageNameValidationResult {
+        return validatePackageNameWorker(packageName, /*supportScopedPackage*/ true);
+    }
+
+    function validatePackageNameWorker(packageName: string, supportScopedPackage: false): NameValidationResult;
+    function validatePackageNameWorker(packageName: string, supportScopedPackage: true): PackageNameValidationResult;
+    function validatePackageNameWorker(packageName: string, supportScopedPackage: boolean): PackageNameValidationResult {
         if (!packageName) {
-            return PackageNameValidationResult.EmptyName;
+            return NameValidationResult.EmptyName;
         }
         if (packageName.length > maxPackageNameLength) {
-            return PackageNameValidationResult.NameTooLong;
+            return NameValidationResult.NameTooLong;
         }
         if (packageName.charCodeAt(0) === CharacterCodes.dot) {
-            return PackageNameValidationResult.NameStartsWithDot;
+            return NameValidationResult.NameStartsWithDot;
         }
         if (packageName.charCodeAt(0) === CharacterCodes._) {
-            return PackageNameValidationResult.NameStartsWithUnderscore;
+            return NameValidationResult.NameStartsWithUnderscore;
         }
         // check if name is scope package like: starts with @ and has one '/' in the middle
         // scoped packages are not currently supported
-        // TODO: when support will be added we'll need to split and check both scope and package name
-        if (/^@[^/]+\/[^/]+$/.test(packageName)) {
-            return PackageNameValidationResult.ScopedPackagesNotSupported;
+        if (supportScopedPackage) {
+            const matches = /^@([^/]+)\/([^/]+)$/.exec(packageName);
+            if (matches) {
+                const scopeResult = validatePackageNameWorker(matches[1], /*supportScopedPackage*/ false);
+                if (scopeResult !== NameValidationResult.Ok) {
+                    return { name: matches[1], isScopeName: true, result: scopeResult };
+                }
+                const packageResult = validatePackageNameWorker(matches[2], /*supportScopedPackage*/ false);
+                if (packageResult !== NameValidationResult.Ok) {
+                    return { name: matches[2], isScopeName: false, result: packageResult };
+                }
+                return NameValidationResult.Ok;
+            }
         }
         if (encodeURIComponent(packageName) !== packageName) {
-            return PackageNameValidationResult.NameContainsNonURISafeCharacters;
+            return NameValidationResult.NameContainsNonURISafeCharacters;
         }
-        return PackageNameValidationResult.Ok;
+        return NameValidationResult.Ok;
     }
 
     export function renderPackageNameValidationFailure(result: PackageNameValidationResult, typing: string): string {
+        return typeof result === "object" ?
+            renderPackageNameValidationFailureWorker(typing, result.result, result.name, result.isScopeName) :
+            renderPackageNameValidationFailureWorker(typing, result, typing, /*isScopeName*/ false);
+    }
+
+    function renderPackageNameValidationFailureWorker(typing: string, result: NameValidationResult, name: string, isScopeName: boolean): string {
+        const kind = isScopeName ? "Scope" : "Package";
         switch (result) {
-            case PackageNameValidationResult.EmptyName:
-                return `Package name '${typing}' cannot be empty`;
-            case PackageNameValidationResult.NameTooLong:
-                return `Package name '${typing}' should be less than ${maxPackageNameLength} characters`;
-            case PackageNameValidationResult.NameStartsWithDot:
-                return `Package name '${typing}' cannot start with '.'`;
-            case PackageNameValidationResult.NameStartsWithUnderscore:
-                return `Package name '${typing}' cannot start with '_'`;
-            case PackageNameValidationResult.ScopedPackagesNotSupported:
-                return `Package '${typing}' is scoped and currently is not supported`;
-            case PackageNameValidationResult.NameContainsNonURISafeCharacters:
-                return `Package name '${typing}' contains non URI safe characters`;
-            case PackageNameValidationResult.Ok:
+            case NameValidationResult.EmptyName:
+                return `'${typing}':: ${kind} name '${name}' cannot be empty`;
+            case NameValidationResult.NameTooLong:
+                return `'${typing}':: ${kind} name '${name}' should be less than ${maxPackageNameLength} characters`;
+            case NameValidationResult.NameStartsWithDot:
+                return `'${typing}':: ${kind} name '${name}' cannot start with '.'`;
+            case NameValidationResult.NameStartsWithUnderscore:
+                return `'${typing}':: ${kind} name '${name}' cannot start with '_'`;
+            case NameValidationResult.NameContainsNonURISafeCharacters:
+                return `'${typing}':: ${kind} name '${name}' contains non URI safe characters`;
+            case NameValidationResult.Ok:
                 return Debug.fail(); // Shouldn't have called this.
             default:
                 throw Debug.assertNever(result);
diff --git a/src/testRunner/unittests/tsserver/typingsInstaller.ts b/src/testRunner/unittests/tsserver/typingsInstaller.ts
@@ -1,6 +1,6 @@
 namespace ts.projectSystem {
     import validatePackageName = JsTyping.validatePackageName;
-    import PackageNameValidationResult = JsTyping.PackageNameValidationResult;
+    import NameValidationResult = JsTyping.NameValidationResult;
 
     interface InstallerParams {
         globalTypingsCacheLocation?: string;
@@ -948,7 +948,8 @@ namespace ts.projectSystem {
                 path: "/a/b/app.js",
                 content: `
                 import * as fs from "fs";
-                import * as commander from "commander";`
+                import * as commander from "commander";
+                import * as component from "@ember/component";`
             };
             const cachePath = "/a/cache";
             const node = {
@@ -959,14 +960,19 @@ namespace ts.projectSystem {
                 path: cachePath + "/node_modules/@types/commander/index.d.ts",
                 content: "export let y: string"
             };
+            const emberComponentDirectory = "ember__component";
+            const emberComponent = {
+                path: `${cachePath}/node_modules/@types/${emberComponentDirectory}/index.d.ts`,
+                content: "export let x: number"
+            };
             const host = createServerHost([file]);
             const installer = new (class extends Installer {
                 constructor() {
                     super(host, { globalTypingsCacheLocation: cachePath, typesRegistry: createTypesRegistry("node", "commander") });
                 }
                 installWorker(_requestId: number, _args: string[], _cwd: string, cb: TI.RequestCompletedAction) {
-                    const installedTypings = ["@types/node", "@types/commander"];
-                    const typingFiles = [node, commander];
+                    const installedTypings = ["@types/node", "@types/commander", `@types/${emberComponentDirectory}`];
+                    const typingFiles = [node, commander, emberComponent];
                     executeCommand(this, host, installedTypings, typingFiles, cb);
                 }
             })();
@@ -980,9 +986,10 @@ namespace ts.projectSystem {
 
             assert.isTrue(host.fileExists(node.path), "typings for 'node' should be created");
             assert.isTrue(host.fileExists(commander.path), "typings for 'commander' should be created");
+            assert.isTrue(host.fileExists(emberComponent.path), "typings for 'commander' should be created");
 
             host.checkTimeoutQueueLengthAndRun(2);
-            checkProjectActualFiles(service.inferredProjects[0], [file.path, node.path, commander.path]);
+            checkProjectActualFiles(service.inferredProjects[0], [file.path, node.path, commander.path, emberComponent.path]);
         });
 
         it("should redo resolution that resolved to '.js' file after typings are installed", () => {
@@ -1263,21 +1270,44 @@ namespace ts.projectSystem {
             for (let i = 0; i < 8; i++) {
                 packageName += packageName;
             }
-            assert.equal(validatePackageName(packageName), PackageNameValidationResult.NameTooLong);
+            assert.equal(validatePackageName(packageName), NameValidationResult.NameTooLong);
+        });
+        it("package name cannot start with dot", () => {
+            assert.equal(validatePackageName(".foo"), NameValidationResult.NameStartsWithDot);
+        });
+        it("package name cannot start with underscore", () => {
+            assert.equal(validatePackageName("_foo"), NameValidationResult.NameStartsWithUnderscore);
+        });
+        it("package non URI safe characters are not supported", () => {
+            assert.equal(validatePackageName("  scope  "), NameValidationResult.NameContainsNonURISafeCharacters);
+            assert.equal(validatePackageName("; say ‘Hello from TypeScript!’ #"), NameValidationResult.NameContainsNonURISafeCharacters);
+            assert.equal(validatePackageName("a/b/c"), NameValidationResult.NameContainsNonURISafeCharacters);
+        });
+        it("scoped package name is supported", () => {
+            assert.equal(validatePackageName("@scope/bar"), NameValidationResult.Ok);
+        });
+        it("scoped name in scoped package name cannot start with dot", () => {
+            assert.deepEqual(validatePackageName("@.scope/bar"), { name: ".scope", isScopeName: true, result: NameValidationResult.NameStartsWithDot });
+            assert.deepEqual(validatePackageName("@.scope/.bar"), { name: ".scope", isScopeName: true, result: NameValidationResult.NameStartsWithDot });
+        });
+        it("scope name in scoped package name cannot start with underscore", () => {
+            assert.deepEqual(validatePackageName("@_scope/bar"), { name: "_scope", isScopeName: true, result: NameValidationResult.NameStartsWithUnderscore });
+            assert.deepEqual(validatePackageName("@_scope/_bar"), { name: "_scope", isScopeName: true, result: NameValidationResult.NameStartsWithUnderscore });
         });
-        it("name cannot start with dot", () => {
-            assert.equal(validatePackageName(".foo"), PackageNameValidationResult.NameStartsWithDot);
+        it("scope name in scoped package name with non URI safe characters are not supported", () => {
+            assert.deepEqual(validatePackageName("@  scope  /bar"), { name: "  scope  ", isScopeName: true, result: NameValidationResult.NameContainsNonURISafeCharacters });
+            assert.deepEqual(validatePackageName("@; say ‘Hello from TypeScript!’ #/bar"), { name: "; say ‘Hello from TypeScript!’ #", isScopeName: true, result: NameValidationResult.NameContainsNonURISafeCharacters });
+            assert.deepEqual(validatePackageName("@  scope  /  bar  "), { name: "  scope  ", isScopeName: true, result: NameValidationResult.NameContainsNonURISafeCharacters });
         });
-        it("name cannot start with underscore", () => {
-            assert.equal(validatePackageName("_foo"), PackageNameValidationResult.NameStartsWithUnderscore);
+        it("package name in scoped package name cannot start with dot", () => {
+            assert.deepEqual(validatePackageName("@scope/.bar"), { name: ".bar", isScopeName: false, result: NameValidationResult.NameStartsWithDot });
         });
-        it("scoped packages not supported", () => {
-            assert.equal(validatePackageName("@scope/bar"), PackageNameValidationResult.ScopedPackagesNotSupported);
+        it("package name in scoped package name cannot start with underscore", () => {
+            assert.deepEqual(validatePackageName("@scope/_bar"), { name: "_bar", isScopeName: false, result: NameValidationResult.NameStartsWithUnderscore });
         });
-        it("non URI safe characters are not supported", () => {
-            assert.equal(validatePackageName("  scope  "), PackageNameValidationResult.NameContainsNonURISafeCharacters);
-            assert.equal(validatePackageName("; say ‘Hello from TypeScript!’ #"), PackageNameValidationResult.NameContainsNonURISafeCharacters);
-            assert.equal(validatePackageName("a/b/c"), PackageNameValidationResult.NameContainsNonURISafeCharacters);
+        it("package name in scoped package name with non URI safe characters are not supported", () => {
+            assert.deepEqual(validatePackageName("@scope/  bar  "), { name: "  bar  ", isScopeName: false, result: NameValidationResult.NameContainsNonURISafeCharacters });
+            assert.deepEqual(validatePackageName("@scope/; say ‘Hello from TypeScript!’ #"), { name: "; say ‘Hello from TypeScript!’ #", isScopeName: false, result: NameValidationResult.NameContainsNonURISafeCharacters });
         });
     });
 
@@ -1309,7 +1339,7 @@ namespace ts.projectSystem {
             projectService.openClientFile(f1.path);
 
             installer.checkPendingCommands(/*expectedCount*/ 0);
-            assert.isTrue(messages.indexOf("Package name '; say ‘Hello from TypeScript!’ #' contains non URI safe characters") > 0, "should find package with invalid name");
+            assert.isTrue(messages.indexOf("'; say ‘Hello from TypeScript!’ #':: Package name '; say ‘Hello from TypeScript!’ #' contains non URI safe characters") > 0, "should find package with invalid name");
         });
     });
 
diff --git a/src/tsserver/server.ts b/src/tsserver/server.ts
@@ -248,7 +248,7 @@ namespace ts.server {
         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) {
+            if (validationResult !== JsTyping.NameValidationResult.Ok) {
                 return false;
             }
 
diff --git a/src/typingsInstallerCore/typingsInstaller.ts b/src/typingsInstallerCore/typingsInstaller.ts
@@ -268,27 +268,28 @@ namespace ts.server.typingsInstaller {
         }
 
         private filterTypings(typingsToInstall: ReadonlyArray<string>): ReadonlyArray<string> {
-            return typingsToInstall.filter(typing => {
-                if (this.missingTypingsSet.get(typing)) {
-                    if (this.log.isEnabled()) this.log.writeLine(`'${typing}' is in missingTypingsSet - skipping...`);
-                    return false;
+            return mapDefined(typingsToInstall, typing => {
+                const typingKey = mangleScopedPackageName(typing);
+                if (this.missingTypingsSet.get(typingKey)) {
+                    if (this.log.isEnabled()) this.log.writeLine(`'${typing}':: '${typingKey}' is in missingTypingsSet - skipping...`);
+                    return undefined;
                 }
                 const validationResult = JsTyping.validatePackageName(typing);
-                if (validationResult !== JsTyping.PackageNameValidationResult.Ok) {
+                if (validationResult !== JsTyping.NameValidationResult.Ok) {
                     // add typing name to missing set so we won't process it again
-                    this.missingTypingsSet.set(typing, true);
+                    this.missingTypingsSet.set(typingKey, true);
                     if (this.log.isEnabled()) this.log.writeLine(JsTyping.renderPackageNameValidationFailure(validationResult, typing));
-                    return false;
+                    return undefined;
                 }
-                if (!this.typesRegistry.has(typing)) {
-                    if (this.log.isEnabled()) this.log.writeLine(`Entry for package '${typing}' does not exist in local types registry - skipping...`);
-                    return false;
+                if (!this.typesRegistry.has(typingKey)) {
+                    if (this.log.isEnabled()) this.log.writeLine(`'${typing}':: Entry for package '${typingKey}' does not exist in local types registry - skipping...`);
+                    return undefined;
                 }
-                if (this.packageNameToTypingLocation.get(typing) && JsTyping.isTypingUpToDate(this.packageNameToTypingLocation.get(typing)!, this.typesRegistry.get(typing)!)) {
-                    if (this.log.isEnabled()) this.log.writeLine(`'${typing}' already has an up-to-date typing - skipping...`);
-                    return false;
+                if (this.packageNameToTypingLocation.get(typingKey) && JsTyping.isTypingUpToDate(this.packageNameToTypingLocation.get(typingKey)!, this.typesRegistry.get(typingKey)!)) {
+                    if (this.log.isEnabled()) this.log.writeLine(`'${typing}':: '${typingKey}' already has an up-to-date typing - skipping...`);
+                    return undefined;
                 }
-                return true;
+                return typingKey;
             });
         }
 

Original file line number	Diff line number	Diff line change
`@@ -248,7 +248,7 @@ namespace ts.server {`
`248`	`248`	`isKnownTypesPackageName(name: string): boolean {`
`249`	`249`	`// We want to avoid looking this up in the registry as that is expensive. So first check that it's actually an NPM package.`
`250`	`250`	`const validationResult = JsTyping.validatePackageName(name);`
`251`		`- if (validationResult !== JsTyping.PackageNameValidationResult.Ok) {`
	`251`	`+ if (validationResult !== JsTyping.NameValidationResult.Ok) {`
`252`	`252`	`return false;`
`253`	`253`	`}`
`254`	`254`