Convert atom-languageclient to TypeScript by daviwil · Pull Request #175 · atom/atom-languageclient

daviwil · 2018-02-07T18:00:22Z

This PR is an experiment to see how much effort it'd be to convert the codebase to TypeScript. Using TypeScript here would help us dogfood the ide-typescript package.

Investigate ownership of official atom typings
Investigate publishing atom-ide typings

daviwil · 2018-02-07T18:01:17Z

lib/convert.ts

 import * as ls from './languageclient';
-import {Point, Range} from 'atom';
+import {Point, ProjectFileEvent, Range, TextEditor} from 'atom';
+import {Diagnostic, DiagnosticType, TextEdit} from '../typings/atom-ide';


Ideally we wouldn't import from ../typings/atom-ide but instead from a "fake" atom-ide module provided by a @types/atom-ide package.

daviwil · 2018-02-07T18:01:48Z

lib/convert.ts

        return ls.DiagnosticSeverity.Information;
      default:
-        (type: empty);
+        // (type: empty);


This line gave an error in the TypeScript compiler, not sure what purpose it serves.

Assuming that the purpose was to assert that the switch was exhaustive over DiagnosticType, one way of writing this in typescript is:

const never: never = type; throw Error(`Unexpected diagnostic type ${never}`);

The never type is a type with no legal values, so that assignment will fail if type inference thinks that there are any legal values for type at that point in the program.

(Using never instead of types in the error ensures that it doesn't look like never is an unused variable when linting or compiling)

daviwil · 2018-02-07T18:03:03Z

typings/atom-ide/index.d.ts

+
+/* declare module 'atom-ide' {*/
+
+  export type OutlineProvider = {


This file is a conversion of the flowtype definitions for Atom-IDE and isn't finished yet. There are some type system differences that I still need to account for.

rictic · 2018-02-08T03:17:26Z

lib/convert.ts

        return ls.DiagnosticSeverity.Information;
      default:
-        (type: empty);
+        // (type: empty);


Assuming that the purpose was to assert that the switch was exhaustive over DiagnosticType, one way of writing this in typescript is:

const never: never = type; throw Error(`Unexpected diagnostic type ${never}`);

The never type is a type with no legal values, so that assignment will fail if type inference thinks that there are any legal values for type at that point in the program.

(Using never instead of types in the error ensures that it doesn't look like never is an unused variable when linting or compiling)

rictic · 2018-02-08T03:19:11Z

lib/convert.ts

  //
  // Returns an {Array} of Atom {TextEdit} objects.
-  static convertLsTextEdits(textEdits: ?Array<ls.TextEdit>): Array<atomIde$TextEdit> {
+  static convertLsTextEdits(textEdits: Array<ls.TextEdit>): Array<TextEdit> {


The TypeScript representation of ?Array<ls.TextEdit> is Array<ls.TextEdit> | null | undefined I think (unless you're not compiling with strict null checks)

Thanks for pointing that out!

rictic · 2018-02-08T03:19:28Z

tsconfig.json

+        "outDir": "build",
+        "lib": ["es6", "dom"],
+        "inlineSources": true,
+        "inlineSourceMap": true,


"strict": true, will catch many more errors, though it also has more false positives

Thanks! I think we'll turn on strictness once we make a bit more progress

daviwil · 2018-02-10T00:06:34Z

Thanks for the help @rictic!

daviwil · 2018-02-14T02:21:58Z

All files are converted now and tests (except for one commented out file) are passing! Next steps are to finish up some small conversion tasks and then start cleaning up the code based on tslint results.

daviwil · 2018-02-14T15:02:25Z

This PR is ready to be reviewed! I'll sit down with @damieng when he's back and go over everything since this is a pretty big change. Everyone else should feel free to ask questions!

damieng · 2018-02-14T15:34:38Z

I'm going to have some time this morning so let's do it then! I'll schedule with you on slack.

daviwil

Adding comments from review with @damieng

daviwil · 2018-02-14T18:32:20Z

lib/adapters/apply-edit-adapter.ts

        // Sort edits in reverse order to prevent edit conflicts.
        edits.sort((edit1, edit2) => -edit1.oldRange.compare(edit2.oldRange));
-        const buffer = editor.getBuffer();
+        const buffer = (editor as any).getBuffer() as TextBuffer;


Need to make this public in Atom APIs

Turns out it is public and in the Atom type defs, fixable on our side

daviwil · 2018-02-14T18:39:38Z

lib/adapters/autocomplete-adapter.ts

-    suggestion.descriptionMarkdown = item.documentation;
+
+    if (typeof item.documentation === 'object') {
+      suggestion.descriptionMarkdown = item.documentation.value;


Check issues to see if this was done based on an issue filed

daviwil · 2018-02-14T18:41:34Z

lib/adapters/code-action-adapter.ts

      return [];
    }
-    invariant(serverCapabilities.codeActionProvider, 'Must have the textDocument/codeAction capability');
+    assert(serverCapabilities.codeActionProvider, 'Must have the textDocument/codeAction capability');


File issue for using assert(canAdapt) more broadly in adapters

daviwil · 2018-02-14T18:45:46Z

lib/adapters/datatip-adapter.ts

+    if (typeof markedString === 'string') {
+      return { type: 'markdown', value: markedString };
+    }
+    else {


Get rid of unnecessary elses

daviwil · 2018-02-14T18:58:40Z

lib/auto-languageclient.ts

+// ChildProcess.  This is used so that language packages with alternative
+// language server process hosting strategies can return something compatible
+// with AutoLanguageClient.startServerProcess.
+export interface LanguageServerProcess extends EventEmitter {


Do this in a separate PR

daviwil · 2018-02-14T19:08:12Z

lib/download-file.ts

 //
 // Returns a {Promise} that will accept when complete.
-export default (async function downloadFile(
+export default (async function DownloadFile(


Change this back to lower case

daviwil · 2018-02-14T19:10:27Z

lib/languageclient.ts

+  }
+}
+
+/**


Move DiagnosticCode here

daviwil · 2018-02-14T19:11:10Z

lib/logger.ts

+  }
+
+  public warn(...args: any[]) {
+    // tslint:disable-next-line:no-console


Can we disable this at file level?

daviwil · 2018-02-14T19:12:08Z

lib/main.ts

-  LinterPushV2Adapter,
-};
+export * from './auto-languageclient';
+export { AutoLanguageClient, DownloadFile, Convert };


daviwil · 2018-02-14T19:16:30Z

package.json

-      }
-    }
-  ],
+  "atomTestRunner": "./build/test/runner",


Make sure this works in Atom test runner

daviwil · 2018-02-14T21:26:18Z

Updated the PR based on feedback and got tests running inside of Atom again. Let me know if there's anything else you'd like me to do before merging!

laughedelic · 2018-02-14T21:50:47Z

lib/adapters/signature-help-adapter.ts


  // Public: Retrieves signature help for a given editor and position.
-  getSignatureHelp(editor: atom$TextEditor, point: atom$Point): Promise<?atomIde$SignatureHelp> {
+  public getSignatureHelp(editor: TextEditor, point: Point): SignatureHelp | Promise<SignatureHelp> {


Shouldn't it be Promise<lsp.SignatureHelp | null>?

Good catch, thanks!

laughedelic · 2018-02-14T23:14:43Z

Could you clarify why all types ?Foo became Foo | null and not Foo | undefined | null or Foo | undefined? (I'm asking because I saw #175 (comment) and read the same somewhere else)

daviwil · 2018-02-14T23:19:12Z

I suppose Foo | undefined | null would be more accurate. I think I misread something in the TypeScript docs when I chose to do it that way. I'll get this fixed soon!

laughedelic · 2018-02-14T23:51:29Z

Cool! I just read about --strictNullChecks and it seem quite useful in this case.

daviwil · 2018-02-14T23:55:45Z

Yep, I'm about 80% of the way through updating the codebase to be checked with that :)

damieng · 2018-02-15T00:15:51Z

We only return null from these methods. I don't think we need undefined.

…

On Wed, Feb 14, 2018, 3:55 PM David Wilson ***@***.***> wrote: Yep, I'm about 80% of the way to updating the codebase to be checked with that :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#175 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAHQp-JlD_TWQZ3b5Z_bbYtx5M56fMZPks5tU3KCgaJpZM4R9I6-> .

daviwil · 2018-02-15T00:21:30Z

Yep, I'm not adding undefined in places where null is returned on purpose to indicate a lack of result.

WIP: TypeScript conversion experiment

7b19d6f

daviwil requested a review from damieng February 7, 2018 18:00

daviwil commented Feb 7, 2018

View reviewed changes

rictic reviewed Feb 8, 2018

View reviewed changes

Continue converting some files to TypeScript

63dfab7

Fix small issues in lib/convert.ts

5c43f88

laughedelic mentioned this pull request Feb 11, 2018

Add support for willSaveWaitUntil request #176

Closed

daviwil added 3 commits February 13, 2018 14:23

Convert more files to TypeScript

6514766

Finish converting library code

428f54d

Convert tests to TypeScript, add tslint

bf1336d

daviwil added 3 commits February 13, 2018 19:31

Remove flow from script/cibuild

92ab456

Re-enable SignatureHelpAdapter tests

c2233e6

Configure tslint rules and update code to conform

22b0bb0

daviwil changed the title ~~WIP: TypeScript conversion experiment~~ Convert atom-languageclient to TypeScript Feb 14, 2018

daviwil added 5 commits February 13, 2018 21:07

Bump tslint severity to error for builds

04056a0

Generate type declaration files for consumers

559b715

Use module augmentation on atom module typings

2c94826

Run tslint on typings folder

dd4c288

Remove Flow, Babel, eslint and their configs

8bf29d4

Improve type definition exports for module consumers

4547634

daviwil commented Feb 14, 2018

View reviewed changes

daviwil added 2 commits February 14, 2018 13:23

Updates based on code review feedback

7745a59

Enable tests to run inside of Atom

b41f817

damieng approved these changes Feb 14, 2018

View reviewed changes

laughedelic reviewed Feb 14, 2018

View reviewed changes

Fix small typing issue in SignatureHelpAdapter

ea1131f

daviwil merged commit 156b306 into master Feb 14, 2018

daviwil deleted the typescript-conversion branch February 14, 2018 22:08

laughedelic mentioned this pull request Feb 24, 2018

Sad state of Flow tooling stereobooster/type-o-rama#3

Closed


		/* declare module 'atom-ide' {*/

		export type OutlineProvider = {

+                }
+              }
+              /**

Conversation

daviwil commented Feb 7, 2018 • edited by damieng Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daviwil commented Feb 10, 2018

Uh oh!

daviwil commented Feb 14, 2018

Uh oh!

daviwil commented Feb 14, 2018

Uh oh!

damieng commented Feb 14, 2018

Uh oh!

daviwil left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daviwil commented Feb 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laughedelic commented Feb 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

daviwil commented Feb 14, 2018

Uh oh!

laughedelic commented Feb 14, 2018

Uh oh!

daviwil commented Feb 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

damieng commented Feb 15, 2018 via email

Uh oh!

daviwil commented Feb 15, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

daviwil commented Feb 7, 2018 •

edited by damieng

Loading

laughedelic commented Feb 14, 2018 •

edited

Loading

daviwil commented Feb 14, 2018 •

edited

Loading