Add quickfix and refactoring to install @types packages #19130

ghost · 2017-10-12T16:13:31Z

Needs testing against VSCode before merging. And probably a lot of code review.

sheetalkamat · 2017-10-12T19:07:04Z

src/compiler/core.ts

@@ -1829,7 +1829,9 @@ namespace ts {
        return i < 0 ? path : path.substring(i + 1);
    }

-    export function combinePaths(path1: string, path2: string) {
+    export function combinePaths(path1: Path, path2: string): Path;


This is not correct.. Path handle case sensitivity.. since path2 doesnt have Path as type you would never be sure if combined paths would be Path or just string (that needs to handle case sensitivity)

sheetalkamat · 2017-10-12T19:09:59Z

src/compiler/moduleNameResolver.ts

@@ -1153,4 +1158,68 @@ namespace ts {
    function toSearchResult<T>(value: T | undefined): SearchResult<T> {
        return value !== undefined ? { value } : undefined;
    }
+
+    /* @internal */
+    export const enum PackageNameValidationResult {


These types shouldnt be in compiler .. They should be in Language service since they arent needed by compiler.

sheetalkamat · 2017-10-12T19:10:11Z

src/compiler/moduleNameResolver.ts

+     * Validates package name using rules defined at https://docs.npmjs.com/files/package.json
+     */
+    /* @internal */
+    export function validatePackageName(packageName: string): PackageNameValidationResult {


These functions too..

mhegazy · 2017-10-12T20:45:31Z

src/server/protocol.ts

@@ -1541,6 +1561,7 @@ namespace ts.server.protocol {
        description: string;
        /** Text changes to apply to each file as part of the code action */
        changes: FileCodeEdits[];
+        commands?: {}[];


we should add a note that this is an opaque object to be passed as the input to the apply command request.

mhegazy · 2017-10-12T20:47:34Z

src/server/server.ts

+                return this.typesRegistryCache;
+            }
+
+            this.send({ kind: "typesRegistry" });


we can be in a state wherte we send the registry multiple times. we should add a check to make sure we only send it once.

mhegazy · 2017-10-12T20:51:20Z

tests/baselines/reference/api/tsserverlibrary.d.ts

@@ -6837,7 +6882,7 @@ declare namespace ts.server {
        logError(err: Error, cmd: string): void;
        send(msg: protocol.Message): void;
        event<T>(info: T, eventName: string): void;
-        output(info: any, cmdName: string, reqSeq?: number, errorMsg?: string): void;
+        output(info: {} | undefined, cmdName: string, reqSeq: number, success: boolean, message?: string): void;


we should not be breaking this public API

mhegazy · 2017-10-12T20:56:25Z

src/services/services.ts

+            fileName = toPath(fileName, currentDirectory, getCanonicalFileName);
+            switch (action.type) {
+                case "install package":
+                    return host.installPackage({ fileName, packageName: action.packageName });


host.installPackage is optional, we should check it before calling it

mhegazy · 2017-10-12T21:00:53Z

src/services/types.ts

@@ -199,6 +202,9 @@ namespace ts {
         * Gets a set of custom transformers to use during emit.
         */
        getCustomTransformers?(): CustomTransformers | undefined;
+
+        tryGetTypesRegistry?(): Map<void> | undefined;


consider changing the API to be isKownTypesPackageName(name: string). the server still gets the registry internally and uses it to answer these requests. but the LS API does not really need to know about this detail.

mhegazy · 2017-10-12T21:07:31Z

src/server/client.ts

@@ -540,6 +540,16 @@ 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 };


leave that as not impmeneted for now.

mhegazy · 2017-10-16T18:43:52Z

src/server/server.ts

+            // 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;


return false?

mhegazy · 2017-10-16T18:51:17Z

src/services/utilities.ts

+    /**
+     * Cheap PromiseLike implementation.
+     */
+    export class PromiseImpl<T> implements PromiseLike<T> {


Why do we need a custom Promise implementation?

node v4.0 seems like a reasonable minimum supported platform for the services..

@anstarovoyt does your engine have Promise support?

we don't use it

I am sorry, do not use what?

I am asking about your runtime, is it node v4 or compatible?

yes, we expect that our users use node >= v4 for running the service

sheetalkamat · 2017-10-17T18:15:57Z

src/server/project.ts

+        /*@internal*/
+        protected getProjectRootPath(): Path | undefined {
+            return this.projectRootPath as Path;
+        }


thiws method isnt needed... There is always projectRootPath set as currentDirectory field on project

OK, do you know why currentDirectory is not a Path?

sheetalkamat · 2017-10-17T18:16:07Z

src/server/project.ts

+        /* @internal */
+        protected getProjectRootPath(): Path | undefined {
+            return this.canonicalConfigFilePath as string as Path;
+        }


This too not needed

sheetalkamat · 2017-10-17T18:16:16Z

src/server/project.ts

+        /* @internal */
+        protected getProjectRootPath(): Path | undefined {
+            return this.projectFilePath as Path;
+        }


and this too..

ghost · 2017-10-17T18:59:46Z

@sheetalkamat Good to go?

ghost · 2017-10-18T17:51:42Z

@mhegazy Since we're using a real Promise maybe it would be a good idea to update type definitions from PromiseLike to Promise?

mhegazy · 2017-10-18T20:09:06Z

yes. Promise is now in lib.d.ts, so should be a non-breaking change.

billti · 2017-10-19T04:47:44Z

Anything needed on VS side for this to work?

ghost · 2017-10-19T15:05:59Z

We would need to implement functionality to call back into the language server after a code action is accepted, if it contains commands. microsoft/vscode#34787 should still be an accurate description. This hasn't yet been implemented in vscode.

* Adding test case where opened file included in project is not added to ref count of configured project * Fix the way configured project's reference is managed so that the open file * Do not watch root folders for failed lookup locations and effective type roots Fixes #19170 * LEGO: check in for master to temporary branch. * LEGO: check in for master to temporary branch. * Mark fresh spread objects w/ContainsObjectLiteral * Test excess property checks of spreads of unions. * Add exported members of all project files in the global completion list (#19069) * checker.ts: Remove null check on symbols * tsserverProjectSystem.ts: add two tests * client.ts, completions.ts, types.ts: Add codeActions member to CompletionEntryDetails * protocol.ts, session.ts: Add codeActions member to CompletionEntryDetails protocol * protocol.ts, session.ts, types.ts: add hasAction to CompletionEntry * session.ts, services.ts, types.ts: Add formattingOptions parameter to getCompletionEntryDetails * completions.ts: define SymbolOriginInfo type * completions.ts, services.ts: Add allSourceFiles parameter to getCompletionsAtPosition * completions.ts, services.ts: Plumb allSourceFiles into new function getSymbolsFromOtherSourceFileExports inside getCompletionData * completions.ts: add symbolToOriginInfoMap parameter to getCompletionEntriesFromSymbols and to return value of getCompletionData * utilities.ts: Add getOtherModuleSymbols, getUniqueSymbolIdAsString, getUniqueSymbolId * completions.ts: Set CompletionEntry.hasAction when symbol is found in symbolToOriginInfoMap (meaning there's an import action) * completions.ts: Populate list with possible exports (implement getSymbolsFromOtherSourceFileExports) * completions.ts, services.ts: Plumb host and rulesProvider into getCompletionEntryDetails * completions.ts: Add TODO comment * importFixes.ts: Add types ImportDeclarationMap and ImportCodeFixContext * Move getImportDeclarations into getCodeActionForImport, immediately after the implementation * importFixes.ts: Move createChangeTracker into getCodeActionForImport, immediately after getImportDeclarations * importFixes.ts: Add convertToImportCodeFixContext function and reference it from the getCodeActions lambda * importFixes.ts: Add context: ImportCodeFixContext parameter to getCodeActionForImport, update call sites, destructure it, use compilerOptions in getModuleSpecifierForNewImport * importFixes.ts: Remove moduleSymbol parameter from getImportDeclarations and use the ambient one * importFixes.ts: Use cachedImportDeclarations from context in getCodeActionForImport * importFixes.ts: Move createCodeAction out, immediately above convertToImportCodeFixContext * Move the declaration for lastImportDeclaration out of the getCodeActions lambda into getCodeActionForImport * importFixes.ts: Use symbolToken in getCodeActionForImport * importFixes.ts: Remove useCaseSensitiveFileNames altogether from getCodeActions lambda * importFixes.ts: Remove local getUniqueSymbolId function and add checker parameter to calls to it * importFixes.ts: Move getCodeActionForImport out into an export, immediately below convertToImportCodeFixContext * completions.ts: In getCompletionEntryDetails, if there's symbolOriginInfo, call getCodeActionForImport * importFixes.ts: Create and use importFixContext within getCodeActions lambda * importFixes.ts: Use local newLineCharacter instead of context.newLineCharacter in getCodeActionForImport * importFixes.ts: Use local host instead of context.host in getCodeActionForImport * importFixes.ts: Remove dummy getCanonicalFileName line * Filter symbols after gathering exports instead of before * Lint * Test, fix bugs, refactor * Suggestions from code review * Update api baseline * Fix bug if previousToken is not an Identifier * Replace `startsWith` with `stringContainsCharactersInOrder` * Dont try to run unit tests with rwc tests again (#19240) * In tsserver, indent logged JSON (#19080) * noUnusedLocals: Warn for recursive call to private method (#18920) * Added test for windows style paths watched directories * Handle when directory watcher is invoked on file change Fixes #19206 * Add quickfix and refactoring to install @types packages (#19130) * Add quickfix and refactoring to install @types packages * Move `validatePackageName` to `jsTyping.ts` * Remove combinePaths overloads * Respond to code review * Update api baselines * Use native PromiseConstructor * Return false instead of undefined * Remove getProjectRootPath * Update api * This wasnt required before... (#19262) * Collapse jsdoc annotation refactors to one Previously there were two, and two always fired. * Update baselines * Fix #19257: Ensure a generated signature has a return type (#19264) * Fix #19257: Ensure a generated signature has a return type * Ensure generated properties have types * Use the same context for multiple inferences to the same property access * Respect newLine compiler option in language service output (#19279) * LEGO: check in for master to temporary branch. * LEGO: check in for master to temporary branch. * LEGO: check in for master to temporary branch. * Disambiguate same-named refactors using description (#19267) Disambiguate same-named refactors using actionName * Set the scriptKind from the host configuration if present * Update API baselines * Remove erroneous error for JSDoc object literals appears with checkJS. * Update annotateWithTypeFromJSDoc tests 1. Object literals are single-line now. 2. Index signatures transform to TS index signatures. 3. The refactoring is only available when it could add types. * Add a better test for jsdoc index signatures. The test case shows that the errorenous error no longer appears. * Fix bugs in jsdoc annotation refactor 1. Transform index signatures to TS index signatures. 2. Print object literals on a single line. 3. Only offer the refactor when it could add types. (There must not be a type annotation already, and there must be a JSDoc that applies.) * Move isJSDocIndexSignature to utilities * Add failing testcase where when d.ts file is in program, the files get emitted multiple times with --out setting * Modify api to emit affected files using callback instead of generating in memory output Also marking few apis introduced during watch improvements changes that are suppose to be internal for now * Use get files affected by internally and hence use file paths as input * Simplify emit changed files further Also use source file version as the signature of declaration file instead of computing it from text * Do not cache the semantic diagnostics when compiler options has --out since we would anyways get all fresh diagnostics * make getCurrentDirectory required (#19303) * LEGO: check in for master to temporary branch. * Actually use cached semantic diagnostics * Fix tsc-instrumented 1. Make recursiveCreateDirectory correctly handle relative paths. 2. Remove dependency on Harness 3. Correctly increment iocapture0, iocapture1, ... iocaptureN. 4. Stop double-nesting baseline files. * Fix lint * Fix #19270: ensure output name is a valid locale name (#19308) * Fix #19270: ensure output name is a valid locale name * Use const instead of var * Add comment * Fix typo * Split the concat logic for generatedLCGFile * findAllRefs: Support anonymous default export (#19302) * Fix undefined error using `getEffectiveTypeRoots` (#19300) * Remove extra blank line in logs (#19307) * Use Promise instead of PromiseLike (#19305) * Workaround for nonnull operator on indexed accesses (#19275) * Quick and dirty workaround * Add third case to show current behavior * Rename variable, replace elaboration from comment with links * Remove some unnecessary `undefined` checks in extractSymbol (#19256) * Fix "noStringLiteral" lint errors (#19310) * LEGO: check in for master to temporary branch. * Rename locale outputs * Update LKG

ghost force-pushed the install_types_fix_2 branch 3 times, most recently from fa83247 to 9418845 Compare October 12, 2017 17:13

Add quickfix and refactoring to install @types packages

76b7be8

ghost force-pushed the install_types_fix_2 branch from 9418845 to 76b7be8 Compare October 12, 2017 17:51

ghost requested a review from sheetalkamat October 12, 2017 17:52

sheetalkamat reviewed Oct 12, 2017

View reviewed changes

Andy Hanson added 2 commits October 12, 2017 12:40

Move validatePackageName to jsTyping.ts

458a6de

Remove combinePaths overloads

ee9f892

mhegazy reviewed Oct 12, 2017

View reviewed changes

ghost force-pushed the install_types_fix_2 branch from f7a2288 to 50b47bb Compare October 13, 2017 21:07

Respond to code review

7c22ad6

ghost force-pushed the install_types_fix_2 branch from 50b47bb to 7c22ad6 Compare October 13, 2017 21:42

mhegazy reviewed Oct 16, 2017

View reviewed changes

Andy Hanson added 2 commits October 16, 2017 14:17

Update api baselines

b2d6b88

Use native PromiseConstructor

8a273ed

mhegazy approved these changes Oct 17, 2017

View reviewed changes

Andy Hanson added 3 commits October 17, 2017 10:16

Merge branch 'master' into install_types_fix_2

d370ad3

Merge branch 'master' into install_types_fix_2

f6b0b01

Return false instead of undefined

52cfe9e

sheetalkamat reviewed Oct 17, 2017

View reviewed changes

mhegazy mentioned this pull request Oct 17, 2017

Add quick fix to install module definitions from @types #14423

Closed

Remove getProjectRootPath

dcb903c

Andy Hanson added 2 commits October 17, 2017 12:01

Update api

ce79a84

Merge branch 'master' into install_types_fix_2

b798d77

sheetalkamat approved these changes Oct 17, 2017

View reviewed changes

ghost merged commit d05443b into master Oct 17, 2017

ghost deleted the install_types_fix_2 branch October 17, 2017 22:04

mhegazy mentioned this pull request Oct 17, 2017

Support yarn in Install @types/<package> code fix #19269

Open

ghost mentioned this pull request Oct 18, 2017

Use Promise instead of PromiseLike #19305

Merged

ghost mentioned this pull request Mar 1, 2018

Convert 'installTypesForPackge' refactor to a suggestion #22267

Merged

microsoft locked and limited conversation to collaborators Jun 14, 2018

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add quickfix and refactoring to install @types packages #19130

Add quickfix and refactoring to install @types packages #19130

ghost commented Oct 12, 2017

sheetalkamat Oct 12, 2017

sheetalkamat Oct 12, 2017

sheetalkamat Oct 12, 2017

mhegazy Oct 12, 2017

mhegazy Oct 12, 2017

mhegazy Oct 12, 2017

mhegazy Oct 12, 2017

mhegazy Oct 12, 2017

mhegazy Oct 12, 2017

mhegazy Oct 16, 2017

mhegazy Oct 16, 2017

mhegazy Oct 16, 2017

mhegazy Oct 16, 2017

anstarovoyt Oct 16, 2017

mhegazy Oct 16, 2017

anstarovoyt Oct 16, 2017

sheetalkamat Oct 17, 2017

ghost Oct 17, 2017

sheetalkamat Oct 17, 2017

sheetalkamat Oct 17, 2017

ghost commented Oct 17, 2017

ghost commented Oct 18, 2017

mhegazy commented Oct 18, 2017

billti commented Oct 19, 2017

ghost commented Oct 19, 2017

Add quickfix and refactoring to install @types packages #19130

Add quickfix and refactoring to install @types packages #19130

Conversation

ghost commented Oct 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Oct 17, 2017

ghost commented Oct 18, 2017

mhegazy commented Oct 18, 2017

billti commented Oct 19, 2017

ghost commented Oct 19, 2017