From 49ba408e4fed08e328dfff2614611c500ee53bb0 Mon Sep 17 00:00:00 2001 From: Sheetal Nandi Date: Tue, 16 Jul 2019 10:14:06 -0700 Subject: [PATCH] Handle scoped package names in typing installer Fixes #32075 --- src/jsTyping/jsTyping.ts | 75 +++++++++++++------ .../unittests/tsserver/typingsInstaller.ts | 64 +++++++++++----- src/tsserver/server.ts | 2 +- src/typingsInstallerCore/typingsInstaller.ts | 29 +++---- 4 files changed, 114 insertions(+), 56 deletions(-) diff --git a/src/jsTyping/jsTyping.ts b/src/jsTyping/jsTyping.ts index c2b2ad3f5b3..172d041cc01 100644 --- 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 index b02adbbd094..79b4f01aa06 100644 --- 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("name cannot start with dot", () => { - assert.equal(validatePackageName(".foo"), PackageNameValidationResult.NameStartsWithDot); + it("package name cannot start with dot", () => { + assert.equal(validatePackageName(".foo"), NameValidationResult.NameStartsWithDot); }); - it("name cannot start with underscore", () => { - assert.equal(validatePackageName("_foo"), PackageNameValidationResult.NameStartsWithUnderscore); + it("package name cannot start with underscore", () => { + assert.equal(validatePackageName("_foo"), NameValidationResult.NameStartsWithUnderscore); }); - it("scoped packages not supported", () => { - assert.equal(validatePackageName("@scope/bar"), PackageNameValidationResult.ScopedPackagesNotSupported); + 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("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("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("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("package name in scoped package name cannot start with dot", () => { + assert.deepEqual(validatePackageName("@scope/.bar"), { name: ".bar", isScopeName: false, result: NameValidationResult.NameStartsWithDot }); + }); + it("package name in scoped package name cannot start with underscore", () => { + assert.deepEqual(validatePackageName("@scope/_bar"), { name: "_bar", isScopeName: false, result: NameValidationResult.NameStartsWithUnderscore }); + }); + 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 index 43dc4638418..a9fbf2f3b6a 100644 --- 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 index df83f1a677c..17dae3b4dcb 100644 --- a/src/typingsInstallerCore/typingsInstaller.ts +++ b/src/typingsInstallerCore/typingsInstaller.ts @@ -268,27 +268,28 @@ namespace ts.server.typingsInstaller { } private filterTypings(typingsToInstall: ReadonlyArray): ReadonlyArray { - 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; }); }