Add optional support for TypeScript files (closes #42) #74

mrmckeb · 2016-05-07T09:05:01Z

No description provided.

msftclas · 2016-05-07T09:05:06Z

Hi @mrmckeb, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

msftclas · 2016-05-07T09:05:06Z

Hi @mrmckeb, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

mrmckeb · 2016-05-07T09:05:22Z

I'd be very interested to see if you have any feedback on this too, thanks!

msftclas · 2016-05-07T09:11:56Z

@mrmckeb, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, MSBOT;

mrmckeb · 2016-05-07T09:26:13Z

eslint/extension.ts

@@ -48,6 +57,7 @@ export function activate(context: ExtensionContext) {

 	context.subscriptions.push(
 		new SettingMonitor(client, 'eslint.enable').start(),
+		new SettingMonitor(client, 'eslint.enableTypeScript').start(),


I couldn't find documentation on SettingMonitor, but this seems to actually disable/enable the extension. Is there a way to restart it upon a setting change?

Looking at this again, I might need to either manage configuration updates in the server itself (connection.onDidChangeConfiguration) and remove this line, or create a new client here on each change. I'd be interested to get your thoughts on what's best.

See my overall comments which provides some suggestions to this problem. We should not do this in the server. The cool think about the settings monitor is that it shuts down the server if enable=false which saves a process.

dbaeumer · 2016-05-13T07:49:42Z

@mrmckeb thanks for providing the PR. I was on vacation and I will look into this beginning of next week.

mrmckeb · 2016-05-14T05:27:54Z

No problem @dbaeumer, I think it needs some work still - so I'm looking forward to getting some thoughts on it.

mrmckeb · 2016-05-26T03:30:53Z

Hi @dbaeumer, have you had a chance to take a look at this yet? I'd like to contribute, but would also like a little direction / feedback before submitting final code.

dbaeumer · 2016-05-26T12:02:21Z

@mrmckeb I actually looked at it beginning of this week but I couldn't find a good answer yet how to support best mixed workspaces (a workspace containing TS and JS files) which is nicely supported with the latest TS release. Instead of keeping my thoughts for myself I should have dumped them here without having a solution. Here is the main difficulty I see: in a workspace with JS and TS files we still need to support to disable eslint on TS files. With the current PR this will not work (it will shutdown ESLint for JS files as well). So what needs to be done is:

either fix the SettingMonitor to support that case. This is in the language client npm module. But I don't see how general useful that would be (passing to settings))
implement a special settings monitor in the ESLint extension and don't use the one from the language client
abstract out some basic functions into an AbstractMonitor in the language client and subclass it in ESLint.

I am actually leaning towards 2.)

dbaeumer · 2016-05-26T12:03:50Z

eslint/extension.ts

@@ -9,6 +9,15 @@ import { workspace, window, commands, Disposable, ExtensionContext } from 'vscod
 import { LanguageClient, LanguageClientOptions, SettingMonitor, RequestType, TransportKind, TextEdit, Protocol2Code } from 'vscode-languageclient';

 export function activate(context: ExtensionContext) {
+	let supportedDocuments = ['javascript', 'javascriptreact'];
+	let supportedFileExtensions = ['c.js', 'c.yaml', 'c.yml', 'c', 'c.json'];


IMO this is not necessary. The eslint config files stay the same regardless whether the linter lints JS or TS files

You're right, it was late and I think I misinterpreted this as the supported files for linting, not configuration. Removed.

dbaeumer · 2016-05-26T12:10:48Z

Added more comments to the 'Files changed'

mrmckeb · 2016-05-29T10:19:28Z

Hi @dbaeumer,

Thanks for your time on this. I've had a look at this tonight, updated a few things as per your notes, and have a few thoughts - I was looking at the documentation for extensions and came across the following, and wondered if we could use that to update (or restart) the extension on the fly?

workspace.onDidChangeConfiguration(event => ...);

The event handler could listen to the event (which unfortunately doesn't include details of what changed), and then check to see if any of the changes affect this extension, and then perform an action if required. I'm not sure if the LanguageClient can be updated, or if it would need to be replaced (and a restart needed).

I'd be interested in your thoughts on this. I too would like a solution that doesn't require a restart of VSCode.

microsoft/vscode-eslint#81 microsoft/vscode-eslint#74 microsoft/vscode-react-native#85 flow/flow-for-vscode#13

dbaeumer · 2016-05-31T08:34:02Z

@mrmckeb yes, to listen to configuration changes and to manage the LanguageClient is what I was trying to suggest is my comment. This is what the SettingsMonitor does as well (https://github.com/Microsoft/vscode-languageserver-node/blob/master/client/src/main.ts)

mrmckeb · 2016-05-31T11:34:29Z

Great, thanks for the @dbaeumer - you were clear enough, I just didn't have the context perhaps. I'll tidy this up over the next few days and let you know when it's ready for review again. If you have any other feedback in the meantime, please let me know.

mrmckeb · 2016-06-01T13:39:54Z

Hi @dbaeumer, I've added a first attempt at a custom implementation of the SettingMonitor, that hopefully meets the needs of this extension - and may be something we can publish/share. Let me know what you think of it.

The extension now seems to be hitting the mark in terms of functionality. The only part I'm not happy about is that it's a little slow when changing supported languages, as the client is being recreated.

dbaeumer · 2016-06-02T09:02:47Z

Hi @mrmckeb Thanks for the updated PR. Reading your comment and looking at the code I think we should extend the LanguageClient to get more dynamic in terms of document selectors it handles. Shutting down and restarting if the range of documents changes is not very cool. Would you be willing to look into this as well. The LanguageClient is here: https://github.com/Microsoft/vscode-languageserver-node

Then a small note on naming: to keep things consistent the settings should be named like this:

eslint.enable.TypeScript
eslint.enable.JavaScript
....

mrmckeb · 2016-06-13T08:10:00Z

Sorry I was away last weekend and haven't had a chance to get back to this until now. I'm looking at vscode-languageserver-node now.

mrmckeb · 2016-07-02T05:52:30Z

Hi @dbaeumer, I wanted to give you a quick update - I'm still struggling to find time for this. I'm hoping I'll have time next weekend. I still want to contribute, I've just had a few things take priority over the last month or so.

dbaeumer · 2016-07-06T20:11:03Z

@mrmckeb don't worry. Please note that I will be out on vacation in July. So I can earliest get back to you with comment in August.

JamesHenry · 2016-08-11T22:09:12Z

Hey @mrmckeb, thanks for driving this! We are starting to make progress with the typescript-eslint-parser and this work is an important complement to that.

@dbaeumer hope you had a good vacation :) what are the outstanding points before this can be merged?

danvk · 2016-08-22T17:22:58Z

I've also been playing around with typescript-eslint-parser and would love to be able to see lint warnings in vscode. Looking forward to getting this in!

JamesHenry · 2016-09-06T11:06:35Z

Looping back to this now that a few weeks have passed. In that time, I am proud to say I have been made a member of the ESLint team, so I would really love to help get this across the line in any way I can. I just want to make sure I am clear on what needs to be done.

@dbaeumer am I right in saying the work on the language server is what is currently blocking this PR microsoft/vscode-languageserver-node#44 (comment)?

mrmckeb · 2016-09-25T04:45:05Z

Hi @JamesHenry, thanks for following this up... it's been on my radar for a long time - but unfortunately I ended up on a very large project (working literally every weekend for over two months) and then went on a much-deserved vacation... so needless to say, I've not touched this - and I do feel a bad about that.

You're right, the language server needs some work, the only way to avoid that is to force the user to restart VSCode upon any changes to their linting settings - which is a little dirty. I found a few hacky ways to work around that, but the only real solution is to rework the language server a little so that it can update when needed. This was my first attempt at working on either of these repositories, so @dbaeumer is definitely the expert here.

Again, I'm very sorry that I haven't been able to follow through on this.

dbaeumer · 2016-09-26T07:35:43Z

@mrmckeb @JamesHenry to make this a nice experience we need to do some work on the language server client. I started that work in the branch but haven't continue on it. I will add it back to my list.

JamesHenry · 2016-09-26T14:42:17Z

Thanks so much for getting back to me @mrmckeb and @dbaeumer!

@mrmckeb it happens! No need to feel guilty.

@dbaeumer Thank you, and is there still some thinking to be done around what that might look like? Or is it just a matter of finding the time to implement a specific plan?

I am very happy to help in any way I can if it is the latter, but I naturally lack the context necessary to take on the former right now

trainerbill · 2016-09-29T22:16:05Z

+1. spent an hour trying to figure out why eslinter was not working with my typescript files... now I know.

dbaeumer · 2016-10-14T13:12:26Z

@mrmckeb I am currently working on making the language server protocol more dynamic. This should allow implementing this very easy. Will ping when this is done.

mrmckeb · 2016-10-22T01:37:38Z

That's great @dbaeumer. I'll finish this off the moment you give the go-ahead.

Thanks for kicking this back off.

JamesHenry · 2016-10-22T11:32:25Z

Awesome, thanks so much guys! I am really excited about this

flying-sheep · 2016-11-04T17:45:58Z

hi @dbaeumer, any news?

JamesHenry · 2016-11-20T11:20:31Z

Is there anything I can do to keep this moving @dbaeumer?

We have now reached 1.0.0 with the official typescript-eslint-parser (with some known caveats), and this is now a pretty major blocker for VSCode users.

How is the work from last month (#74 (comment)) progressing? Would extra contributors help?

Many thanks!

dbaeumer · 2016-11-21T17:40:15Z

@JamesHenry thanks for asking. I will finished the work on the LSP in the next day to support adding additional file types to lint on the fly which makes implementing this in ESLint very very easy. May be I will take it as an example.

JamesHenry · 2016-11-21T22:57:09Z

That's fantastic news, I can't wait to try it out! :)

dbaeumer · 2016-12-05T11:37:01Z

Closing this PR. The work is tracked in #21 #42 and I am looking for volunteers to test this :-)

JamesHenry · 2016-12-05T11:49:12Z

Yay! Initial test successful :)

Thanks so much! I will continue to use it in more substantial projects and report any issues on #42

trainerbill · 2016-12-05T19:27:32Z

@JamesHenry Do you have instructions on how you got this working? I am new to vscode.

Edit: I have it installed just not sure how to run the extension.

mrmckeb · 2016-12-05T22:20:31Z

Thanks @dbaeumer, this is great. I'll check it out!

JamesHenry · 2016-12-05T22:37:27Z

@trainerbill I simply followed the steps outlined by @dbaeumer here #42 (comment)

Venryx · 2017-04-25T19:42:46Z

@mrmckeb If you're looking for complete config files for typescript eslint'ing, the following is mine.

Extension: ESLint

[project root]/package.json: (you'll have to run npm install after adding these)

{
    devDependencies: {
        "babel-eslint": "^7.1.1",
        "eslint": "^3.9.1",
        "eslint-config-standard": "^6.2.1",
        "eslint-config-standard-react": "^4.2.0",
        "eslint-plugin-babel": "^3.3.0",
        "eslint-plugin-promise": "^3.3.1",
        "eslint-plugin-react": "^6.0.0",
        "eslint-plugin-standard": "^2.0.0",
        "typescript": "^2.2.2",
        "typescript-eslint-parser": "^2.1.0",
    }
}

[project root]/.eslintrc:

{
	//"parser": "babel-eslint",
	"parser": "typescript-eslint-parser",
	"env": {
		"browser": true,
		"commonjs": true,
		"es6": true,
		"node": true
	},
	"parserOptions": {
		"ecmaVersion": 8,
		"ecmaFeatures": {
			"jsx": true
		},
		"sourceType": "module"
	},
	/*"extends": [
		"standard",
		"standard-react"
	],*/
	"plugins": [
		"babel"
	],
	"globals" : {
		"__DEV__": false,
		"__PROD__": false,
		"__DEBUG__": false,
		"__COVERAGE__": false,
		"__BASENAME__": false
	},
	"rules": {
		"no-const-assign": "warn",
		"no-this-before-super": "warn",
		//"no-undef": "warn",
		"no-unreachable": "warn",
		//"no-unused-vars": "warn",
		"constructor-super": "warn",
		"valid-typeof": "warn",

		//"max-len": [2, 120, 2],
		//"generator-star-spacing": 0,
		//"babel/generator-star-spacing": 1,
		"quotes": ["warn", "backtick"],
		"jsx-quotes": ["warn", "prefer-double"],
		"semi": ["error", "always"],
		//"indent": ["error", "tab", {"ObjectExpression": "off"}],
		"no-mixed-spaces-and-tabs": 1,
		"curly": ["warn", "multi-line"]
	}
}

[project root]/.vscode/settings.json

{
	"typescript.tsdk": "./node_modules/typescript/lib",
	"eslint.enable": true
}

User settings:

{
	"eslint.validate": [
		"javascript",
		"javascriptreact",
		"typescript",
		"typescriptreact"
		//{"language": "html", "autoFix": true},
	],
	"eslint.options": {"extensions": [".js", ".jsx", ".ts", ".tsx"]}
}

msftclas added the cla-required label May 7, 2016

msftclas added cla-signed and removed cla-required labels May 7, 2016

mrmckeb reviewed May 7, 2016
View reviewed changes

dbaeumer reviewed May 26, 2016
View reviewed changes

mrmckeb added 2 commits May 29, 2016 18:22

Add optional support for TypeScript files (closes #42)

ed7e88c

Add option to enable JavaScript, HTML support

6a45415

mrmckeb mentioned this pull request May 29, 2016

Support to lint files of type 'flow' #81

Closed

Add optional support for Flow files (closes #81)

f084204

niedzielski added a commit to niedzielski/sound-of-water that referenced this pull request May 30, 2016

Require JSDocs until Flow has better integration

06ac9a0

microsoft/vscode-eslint#81 microsoft/vscode-eslint#74 microsoft/vscode-react-native#85 flow/flow-for-vscode#13

Added custom SettingMonitor implementation

db2566d

mrmckeb mentioned this pull request Jun 13, 2016

Add ability to change documentSelector dynamically microsoft/vscode-languageserver-node#44

Closed

dbaeumer closed this Dec 5, 2016

Add optional support for TypeScript files (closes #42) #74

Add optional support for TypeScript files (closes #42) #74

Conversation

mrmckeb commented May 7, 2016

msftclas commented May 7, 2016

msftclas commented May 7, 2016

mrmckeb commented May 7, 2016

msftclas commented May 7, 2016

mrmckeb May 7, 2016

Choose a reason for hiding this comment

mrmckeb May 7, 2016

Choose a reason for hiding this comment

dbaeumer May 26, 2016

Choose a reason for hiding this comment

dbaeumer commented May 13, 2016

mrmckeb commented May 14, 2016

mrmckeb commented May 26, 2016

dbaeumer commented May 26, 2016

dbaeumer May 26, 2016

Choose a reason for hiding this comment

mrmckeb May 29, 2016

Choose a reason for hiding this comment

dbaeumer commented May 26, 2016

mrmckeb commented May 29, 2016

dbaeumer commented May 31, 2016

mrmckeb commented May 31, 2016

mrmckeb commented Jun 1, 2016

dbaeumer commented Jun 2, 2016

mrmckeb commented Jun 13, 2016

mrmckeb commented Jul 2, 2016

dbaeumer commented Jul 6, 2016

JamesHenry commented Aug 11, 2016

danvk commented Aug 22, 2016

JamesHenry commented Sep 6, 2016 • edited Loading

mrmckeb commented Sep 25, 2016

dbaeumer commented Sep 26, 2016

JamesHenry commented Sep 26, 2016

trainerbill commented Sep 29, 2016

dbaeumer commented Oct 14, 2016

mrmckeb commented Oct 22, 2016

JamesHenry commented Oct 22, 2016

flying-sheep commented Nov 4, 2016

JamesHenry commented Nov 20, 2016

dbaeumer commented Nov 21, 2016

JamesHenry commented Nov 21, 2016

dbaeumer commented Dec 5, 2016

JamesHenry commented Dec 5, 2016

trainerbill commented Dec 5, 2016 • edited Loading

mrmckeb commented Dec 5, 2016

JamesHenry commented Dec 5, 2016

Venryx commented Apr 25, 2017

JamesHenry commented Sep 6, 2016 •

edited

Loading

trainerbill commented Dec 5, 2016 •

edited

Loading