Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update to TypeScript 2.9.x #1344

Merged
merged 3 commits into from
Jun 5, 2018
Merged

Conversation

rkeithhill
Copy link
Contributor

@rkeithhill rkeithhill commented Jun 2, 2018

PR Summary

Update to latest version of TypeScript.
Update VSCode pkgs.
Update VSCode category list to use updated category name:
"Programming Languages"

The update to TS caused an error in DocumentFormatter but a TS
code action suggested a fix which I applied.

Take advantage of string enum in settings class for HelpCompletion.

PR Checklist

Note: Tick the boxes below that apply to this pull request by putting an x between the square brackets.
Please mark anything not applicable to this PR NA.

  • PR has a meaningful title
  • Summarized changes
  • This PR is ready to merge and is not work in progress
    • If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready

This caused an error in DocumentFormatter but the TS code action
suggested a fix which I applied.

Take advantage of string enum in settings class for HelpCompletion.
@@ -37,22 +37,22 @@
"onCommand:PowerShell.RestartSession"
],
"dependencies": {
"vscode-languageclient": "3.3.0"
"vscode": "^1.1.18",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How come this was moved to being a dependency vs dev dependency?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be the cause of that unfriendly error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is based on the VSCode tutorial on how to write a language server - https://code.visualstudio.com/docs/extensions/example-language-server

There is a section that displays the dependencies you would have in your package.json file as:

"dependencies": {
    "vscode": "^1.1.5",
    "vscode-languageclient": "^3.3.0"
}

RE that error - no, that was pilot error on my part because I forgot that you have to run npm run postinstall right after you run npm install and before you try to run npm run compile.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm I wonder if that was just to make things simple. Even vscode-languageclient itself has vscode as a dev dependency:
https://www.npmjs.com/package/vscode-languageclient

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could remove vscode and see if it still compiles. Want me to do that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that the vscode dependency is necessary, in addition to vscode-languageclient. Without the former I get this error at the end of npm install:

Error: Cannot find module 'C:\Users\Keith\GitHub\rkeithhill\vscode-powershell\node_modules\vscode\bin\install'


// There is something about having this code in the calling method that causes a TS compile error.
// It causes the following error:
// Type 'import("C:/Users/Keith/GitHub/rkeithhill/vscode-powershell/node_modules/vscode-languageserver-typ...'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the issue you were hitting that you fixed with npm run postinstall?

Copy link
Contributor Author

@rkeithhill rkeithhill Jun 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, that was another issue. The error documented in the comment above happened when I simply changed from TS 2.3.x to 2.4.x - bam I'd get that error. My best guess is that newer versions of TS detect the issue. The nice thing about TS 2.9.1 was that it had a code fix for the issue - which I applied above.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried playing around with this just to see if the type error meant anything... Between vscode, vscode-languageclient and vscode-languageserver-types there seems to be a whole can of worms that I can't find much documentation of. So I think this solution is the most tenable for now.

@rjmholt
Copy link
Contributor

rjmholt commented Jun 5, 2018

I've been trying to checkout this branch so I can run it, but haven't been having much luck for the past couple of days

@rkeithhill
Copy link
Contributor Author

When you get time, do not forget to run npm run postinstall after npm install. :-) Also, I recommend nuking node_modules first.

@rjmholt
Copy link
Contributor

rjmholt commented Jun 5, 2018

I will! For now my issue is that git can't find the branch... (I've tried a few ways, the first being the commandline instructions at the bottom)

@rkeithhill
Copy link
Contributor Author

Oh that's weird. I can usually do a git fetch --all and then checkout other folks' branches.

@rjmholt
Copy link
Contributor

rjmholt commented Jun 5, 2018

Aha! The --all found it. None of adding you as a remote, cloning the fork as a separate repo, using GitHub's strange PR ID system worked...

@rkeithhill
Copy link
Contributor Author

rkeithhill commented Jun 5, 2018

I'm far from a Git guru but I use --all because I have the repo forked and this will fetch from both origin and what I call upstream (PowerShell/vscode-powershell). By default, a get fetch/pull only gets updates from origin which is typically your fork - again if you have forked the repo.

@rjmholt
Copy link
Contributor

rjmholt commented Jun 5, 2018

Yeah, I mean I'm on a fork, but I followed a StackOverflow post to add your fork as another remote (called rkeithhill) and ran git fetch rkeithhill. Couldn't work out why that didn't work. But anyway, resolved now!

@rjmholt
Copy link
Contributor

rjmholt commented Jun 5, 2018

Anyone hit anything similar to this:

Version 2.9.1
node_modules/vscode-languageclient/lib/client.d.ts:1:680 - error TS2305: Module ''vscode'' has no exported member 'CodeAction'.

1 import { TextDocumentChangeEvent, TextDocument, Disposable, OutputChannel, FileSystemWatcher as VFileSystemWatcher, DiagnosticCollection, Diagnostic as VDiagnostic, Uri, ProviderResult, CancellationToken, Position as VPosition, Location as VLocation, Range as VRange, CompletionItem as VCompletionItem, CompletionList as VCompletionList, SignatureHelp as VSignatureHelp, Definition as VDefinition, DocumentHighlight as VDocumentHighlight, SymbolInformation as VSymbolInformation, CodeActionContext as VCodeActionContext, Command as VCommand, CodeLens as VCodeLens, FormattingOptions as VFormattingOptions, TextEdit as VTextEdit, WorkspaceEdit as VWorkspaceEdit, Hover as VHover, CodeAction as VCodeAction, DocumentLink as VDocumentLink, TextDocumentWillSaveEvent, WorkspaceFolder as VWorkspaceFolder, CompletionContext as VCompletionContext } from 'vscode';
                                                                                     ~~~~~~~~~~


node_modules/vscode-languageclient/lib/protocolConverter.d.ts:58:45 - error TS2694: Namespace ''vscode'' has no exported member 'CodeAction'.

58     asCodeAction(item: ls.CodeAction): code.CodeAction;
                                               ~~~~~~~~~~


node_modules/vscode-languageclient/lib/protocolConverter.d.ts:60:64 - error TS2694: Namespace ''vscode'' has no exported member 'CodeAction'.

60     asCodeAction(item: ls.CodeAction | undefined | null): code.CodeAction | undefined;
                                                                  ~~~~~~~~~~

@rkeithhill
Copy link
Contributor Author

I got similar errors when I did not execute npm run postinstall before I tried to compile with npm run compile.

@rjmholt
Copy link
Contributor

rjmholt commented Jun 5, 2018

Just ran it again and made sure I ran npm run postinstall. What's the sequence of commands I should be using?

@rkeithhill
Copy link
Contributor Author

My sequence for such an update is:

ri node_modules -r -for
npm install
npm run postinstall
npm run compile

After that, you can use ctrl+shift+b to compile

@rjmholt
Copy link
Contributor

rjmholt commented Jun 5, 2018

Okie doke -- I think I ran npm install the first time and it changed package.json. I reset to HEAD and ran the steps and it works very nicely.

Copy link
Contributor

@rjmholt rjmholt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@rkeithhill
Copy link
Contributor Author

Yeah, I had started this almost a month ago thinking - should be easy - and ran into the TextEdit multiple definition mess. I messed around with it on/off for a while and tabled it. Started up again played around with minimizing the number of npm pkg updates. That's when I noticed simply moving from TS 2.3.x to 2.4.x surfaced this error message.

It seems to me there is one def of TextEdit (and friends) you get with the vscode pkg and a slightly different TextEdit you get with vscode-languageclient. It wasn't until I went to TS 2.9.x that I noticed the code-fix light-bulb, invoked it and voila - compiler error went away.

I'm still frankly puzzled by the error. The best I can figure is that the use of DocumentRangeFormattingRequest.type (brings in TextEdit def from vscode-languageclient) in the same method that uses vscode's TextEdit causes the problem. But I wouldn't bet $2 on it. :-)

@rjmholt
Copy link
Contributor

rjmholt commented Jun 5, 2018

Yeah I'm imagining they've done something strange like defined a type twice and now there some version-dependent cross pollution that really isn't helped at all by the whole node_modules and node dependency management phenomenon. But unless it causes us any real problems, it doesn't feel worth trying to tackle.

Copy link
Member

@TylerLeonhardt TylerLeonhardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM :) thanks @rkeithhill!

@rkeithhill rkeithhill merged commit 6c96a2d into master Jun 5, 2018
@rkeithhill rkeithhill deleted the rkeithhill/update-typescript-version branch June 5, 2018 21:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants