Make a preview of the tslint plugin available to VS Code users

[egamma]

This issue tracks what needs to be done before we can announce an early preview for VS Code users.

• [x] Document the setup for VS Code (e.g. disable vscode-tslint, npm install the plugin and the desired tslint version)
• [x] Plugin must work with both tslint 4 and tslint 5 (I don't think early adopters are still using tslint3 but it would better to support all versions).
• [x] Plugin must work with TS 2.3.1.

Blocking issues:

• [x] Microsoft/TypeScript#15237 TSServer Protocol.d.ts Diagnostic Interface Does Not have a Category Field will be in 2.3.1
• [x] Microsoft/TypeScript#15224 Support to distinguish diagnostics from TS from the diagnostics from a language server plugin will be in 2.3.1
• [x] Open tslint.json quick fix doesn't work in VS Code (works in Eclipse). removed the code action for now

[angelozerr]

TSServer Protocol.d.ts Diagnostic Interface Does Not have a Category Field Without this fix all tslint rule failures show up as errors.

To fix this problem, I call semanticDiagnostics with "includeLinePosition" to true which use "category" https://github.com/Microsoft/TypeScript/blob/v2.2.2/src/server/protocol.ts#L357

Here a demo:

Open tslint.json quick fix doesn't work in VS Code (works in Eclipse).

The codefix is very simply:

{
    "description": "Open tslint.json",
    "changes": [
        {
            "fileName": "C:\\Users\\xxx\\tslint.json",
            "textChanges": []
        }
    ]
}

Perhaps VSCode ignore this codefix because textChanges is empty? Perhaps VSCode cannot apply a codefix for a given file different from the file which execute codefix?

[egamma]

To fix this problem, I call semanticDiagnostics with "includeLinePosition" to true which use "category"

I assume you make this call on the eclipse side? There is still the issue that category is missing from the protocol.ts which we use as the spec for vscode. Anyway, this issue is fixed by now.

Regarding the 'open tslint.json' code fix. This is a protocol spec issue. A code action with an empty textChange can be handled in two ways 1) it can be ignored - this is what VS Code does or 2) the file is opened but no change is made. Since the behaviour is not specified in the protocol, your code action cannot rely on a particular behaviour and therefore the implementation is not incorrect. We could fix this in VS Code and align the implementation with the one in eclipse, but when the next client comes along, then it can have the same issue.

[angelozerr]

@egamma if I undesrtand, we should:

• remove "open tslint.json" code fix. I had done that to start supporting https://github.com/angelozerr/tslint-language-service/issues/4 I think it's a very cool feature
• use existing error code and not use registerCodeFix.

Is that?

[angelozerr]

@egamma I have published 0.7.0 which removes "open tslint.json" code fix and use existing error code in order to there is no pb with VSCode.

Tell me if it's OK.

[egamma]

@angelozerr yes that's it, thanks!

remove "open tslint.json" code fix. I had done that to start supporting #4 I think it's a very cool feature

Agreed it is a nice feature and we should not forget about it. I suggest that we keep a backlog in an issue to start tracking the future work. I've checked off this item.

[egamma]

@angelozerr regarding tslint4 and tslint5 support could you already verify that 0.7.0 supports both versions?

[egamma]

@angelozerr I've tried 0.7.0 in VS Code and it works fine. The setup is:

• cd workspace folder
• npm install tslint-language-service
• npm install tslint@4.5.1 (peer dependency is defined as 4.x)
• open the workspace folder in VS Code
• disable vscode-tslint for the workspace
• edit the tsconfig.json
• make sure you use the TS 2.2.2 version (it doesn't yet work with TS 2.3.next)

  {
  "compilerOptions": {
      "plugins": [
          {
              "name": "tslint-language-service"
          }
      ]
  }
  }

[angelozerr]

@angelozerr regarding tslint4 and tslint5 support could you already verify that 0.7.0 supports both versions?

Few tests I have done, seems working with both version.

I've tried 0.7.0 in VS Code and it works fine.

Very cool news!

[angelozerr]

@egamma I have added your note about VSCode at https://github.com/angelozerr/tslint-language-service#vscode

[egamma]

@angelozerr there is one gotcha in I've only got this all working when using TS 2.2 from inside VS Code. Code fixes don't work when using TS 2.3 that will be included in VS Code 1.12 that we ship next week.

[egamma]

@angelozerr I've met with the TS team and the good news is that both Microsoft/TypeScript#15237 and Microsoft/TypeScript#15224 will be in TS 2.3.1

[angelozerr]

Code fixes don't work when using TS 2.3

Problem should be fixed https://github.com/angelozerr/tslint-language-service/commit/506b751c7911780ff8f0171e022a0aa53e2b5ee4

Please install 0.9.0 and tell me if it works for you.

I've met with the TS team and the good news is that both Microsoft/TypeScript#15237 and Microsoft/TypeScript#15224 will be in TS 2.3.1

Very cool news!

[egamma]

@angelozerr I've installed 0.9.0 and Code fixes are now working, this is good news.

However, I'm now seeing an assertion failure in the tsserver after a quick fix has been applied. The assertion is that a ScriptInfo file is missing.

scriptInfo for a file '...typescript/lib/lib.d.ts' is missing

I haven't seen this failure in 2.2.2 and it is not clear whether it is related to the plugin. The assertion failure does not occur when the plugin is not installed. Need to investigate this further.

[angelozerr]

The assertion is that a ScriptInfo file is missing.

Cannot reproduce this problem -(

Is this problem occurs for any codefix (tslint and TypeScript)?

[egamma]

Cannot reproduce this problem -(

which version of the tsserver are you using? Are you using the tsserver from eclipse or VS Code? I'm using version 2.3.1-insiders.20170420.

This is what I'm seeing, notice that the tsserver appears to hang (hover and intellisense no longer respond).

The problem is not related to the quick fixes, it still occurrs when I comment out the code fixing in the plugin.

I can make the problem go away when uncommenting the call to getProgram() here https://github.com/angelozerr/tslint-language-service/blob/master/src/index.ts#L182

Obviously this turns the linting into a no-op and doesn't produce any errors.

[egamma]

Also what version of tslint are you using. I was using tslint@5.1.0

[angelozerr]

I had tested inside Eclipse with :

• typescript@2.3.1-insiders.20170420
• tslint@5.1.0
• tslint-language-service@0.9.0

and node v6.9.4

It works well: I have codefix and none hangs?

Could you share your little project please.

[egamma]

Good to now that it works for eclipse. Here is the current vscode testing matrix:

tslint-language-server@0.9.0, tslint@4.5.1, typescript@2.2.2 -> OK tslint-language-server@0.9.0, tslint@4.5.1, typescript@2.3.0-dev.20170424 -> OK tslint-language-server@0.9.0, tslint@4.5.1, typescript@2.3.1-insiders.20170420 -> OK tslint-language-server@0.9.0, tslint@5.1.0, typescript@2.2.2 -> Not OK, tsserver hang tslint-language-server@0.9.0, tslint@5.1.0, typescript@2.3.0-dev.20170424 -> Not OK, tsserver hang tslint-language-server@0.9.0, tslint@5.1.0, typescript@2.3.1-insiders.20170420 -> Not OK, tsserver hang

The hangs always happen with tslint@5.1.0.

[egamma]

I doubt it is the test project but here it is git@github.com:egamma/test-ts-server-plugin.git

[angelozerr]

I can reproduce your problem now with Eclipse. Thanks for sharing your project.

With my quick test, it seems that there is a tslint rules which causes this problem.

If I use this tslint.json rules:

"rules": {
        "quotemark": [
            true,
            "double"
        ]
    },

It seems working well.

[egamma]

I suspect the rules which rely on the 'project'/type checker are causing a side effect... this is one of main selling points for the plugin...

This is from the tslint 5.0 release notes: https://github.com/palantir/tslint/releases/tag/5.0.0 Some rules that previously worked without the type checker now require it. This includes: no-unused-variable no-use-before-declare

[angelozerr]

I suspect the rules which rely on the 'project'/type checker are causing a side effect..

It means that tslint 5 cannot be used with tslint-language-service and vscode-tslint?

[egamma]

It means that tslint 5 cannot be used with tslint-language-service and vscode-tslint?

We need to look deeper into why a tslint5 rule that requires a typecheck will cause the assertion failure in the typescript server. I'll do a minimal test case and file an issue against typescript.

[egamma]

This is the issue with reproducable steps: https://github.com/Microsoft/TypeScript/issues/15344

[angelozerr]

This is the issue with reproducable steps: Microsoft/TypeScript#15344

Thanks to have filling this issue. I tell me if we should ignore codefix with rules (which uses typechecker) which breaks TypeScript by waiting a fix from TypeScript or tslint. I mean:

• ignore rules which uses typeschecker
• by checking version of tslint and TypeScript

What do you think about that?

[egamma]

Having support for rules that need type checking is one of the main selling points for running tslint as a language server plugin vs. a separate language server, so I don't want to give up on this yet. I see that for Eclipse the current support is already a win.

ignore rules which uses typeschecker

this could be done by not passing in the Program to the linter and only pass the contents of the current file (same as what vscode-tslint does). When there is no Program then tslint will not run the rules that need typechecking.

One option would be to enable the full checking using a plugin setting in the tsconfig.json. This would allow us to continue the debugging the full support and Eclipse users could already profit from single file linting.

by checking version of tslint and TypeScript

The current issue is not tslint version specific. Tslint 4.5 already supports rules that need type checking and they will trigger the assertion failure. The reason why it looked like a tslint 5 issue during the testing https://github.com/angelozerr/tslint-language-service/issues/15#issuecomment-296560825 is that tslint 5 changed the implementation the rule no-unused-variable so that it now uses typechecking.

[angelozerr]

But perhaps we could ignore the rule no-unused-variable for tslint5?

Otherwise what is the solution of this issue?

[egamma]

But perhaps we could ignore the rule no-unused-variable for tslint5?

there are other rules which require typechecking (see screenshot below) we should not hard code any rule names, since users can extend tslint with additional rules.

Otherwise what is the solution of this issue?

If we want to solve the issue now, then we can implement what I suggested above, that is to not pass the Program to the linter and tslint will not run the rules that require a typecheck. This would be change:

          let tslint = new linter(options/*, <any>oldLS.getProgram()*/);
          tslint.lint(fileName, oldLS.getProgram().getSourceFile(fileName).getFullText(), configuration);

[angelozerr]

If we want to solve the issue now, then we can implement what I suggested above, that is to not pass the Program to the linter

If we do that, it means that it will be less performant? It means that TypeScript file will be parsed twice?

If it that, perhaps we could do a test by using your matrice:

tslint-language-server@0.9.0, tslint@4.5.1, typescript@2.2.2 -> OK
tslint-language-server@0.9.0, tslint@4.5.1, typescript@2.3.0-dev.20170424 -> OK
tslint-language-server@0.9.0, tslint@4.5.1, typescript@2.3.1-insiders.20170420 -> OK
tslint-language-server@0.9.0, tslint@5.1.0, typescript@2.2.2 -> Not OK, tsserver hang
tslint-language-server@0.9.0, tslint@5.1.0, typescript@2.3.0-dev.20170424 -> Not OK, tsserver hang
tslint-language-server@0.9.0, tslint@5.1.0, typescript@2.3.1-insiders.20170420 -> Not OK, tsserver hang

When tsserver hangs, we use text of the Script file otherwise we use the program. What do you think?

and tslint will not run the rules that require a typecheck. This would be change:

Could you do a PR for that?

[egamma]

If we do that, it means that it will be less performant? It means that TypeScript file will be parsed twice?

good point, in this case tslint will parse the TS file again, which removes the benefit of running tslint as a plugin 😞

Therefore my conclusion is to not try to fix this now with a workaround, but we have to wait and investigate more into Microsoft/TypeScript#15344.

[egamma]

@angelozerr I've tracked down the issue pls see https://github.com/Microsoft/TypeScript/issues/15344#issuecomment-297386000

It turns out that the noUnusedVariable rule does something strange in the implementation. Since the TS compiler provide a compiler option for this rule this can easily be worked around. So we could mention in the release notes that the noUnusedVariable rule must not be used.

[angelozerr]

So we could mention in the release notes that the noUnusedVariable rule must not be used.

Yes and perhaps remove it once tslint config is loaded (I don't know if it's possible)?

Perhaps we should create an issue to tslint?

[egamma]

Yes and perhaps remove it once tslint config is loaded (I don't know if it's possible)?

Currently tslint reads the configuration, tweaking it would require tweaking the tslint.json config from the user, which I'd prefer not doing.

Perhaps we should create an issue to tslint?

absolutely but I'd like to get insights from the TS team and what would be the proper fix for this rule.

[egamma]

I've implemented a work around and remove the no-unused-variable rule under tslint5 as suggested by you above. PR follows.

[angelozerr]

Wow very cool!

After this accept of this PR, I think we could publish to 1.0.0 What do you think @egamma ?

[egamma]

I made some more tweaks to the README and submitted another PR.

Please publish another version. Given the amount of testing we have done up to now I'd suggest to go with 0.9.2 for now.

[egamma]

@angelozerr the plugin is mentioned in the TypeScript 2.3 release blog

[angelozerr]

Very cool news, thanks for sharing this info.

0.9.2 is now published.

[angelozerr]

I think we can close this issue?

[egamma]

yep - closed 🎉🎉