Add support for TypeScript and document usage with Flow #326

jorgegonzalez · 2018-05-17T23:52:12Z

Fixes #315

Allows TypeScript and Flow files to be parsed by XO by setting "flow": true or "typescript": true in the config.

I think the error messages can be improved a bit. And I'm not 100% sure if that's where this particular block of code should go, but it works.

Also there is probably a better way to get the devDependencies list.

pvdlg · 2018-05-18T09:02:47Z

Instead of looking into the dev dependencies could we do something like that?

Add ts and tsx to the default extension list we parse
For files with a .ts or .tsx extension automatically set the parser to typescript-eslint-parser
Add a paragraph to the documentation explaining that in order to use flow, the parser must must be set to babel-parser (as we don't have a way to detect if a file is written in Flow or regular JS and set the parser automatically)

That would avoid adding the typescript and flow config options which are not really useful. That would also make things works out of the box for users (at least in the case of typescript).

jorgegonzalez · 2018-05-18T13:30:48Z

I agree.

jorgegonzalez · 2018-05-18T13:33:16Z

XO will show these errors if the proper dependencies are not installed:

pvdlg · 2018-05-18T13:37:07Z

index.js

@@ -102,6 +102,10 @@ module.exports.lintFiles = (patterns, opts) => {
 			});
 		}

+		if (paths.filter(path => path.includes('.ts')).length > 0) {


You should use path.extname here instead of includes. I guess it should also includes .tsx files as well.

pvdlg · 2018-05-18T13:39:19Z

Ultimately we might consider using TSLint rather (or in combination of) to lint .ts and .tsx files but that would be another issue/PR/discussion.

jorgegonzalez · 2018-05-18T14:05:05Z

There is a benefit in applying ESLint rules to TypeScript though, as TSLint is missing many of the nicer ESLint plugins (like eslint-plugin-unicorn), and it can be a little wonky in my experience.

pvdlg · 2018-05-18T14:12:02Z

Indeed. That would probably be a combination of both.

jorgegonzalez · 2018-05-21T20:01:38Z

Hmmm getting an import/no-unresolved error for .ts files. But only if the extension is excluded in the import:

import test from './test'; //=> error
import test from './test.ts'; //=> no error

sindresorhus · 2018-05-21T20:32:27Z

readme.md

+To enable parsing of Flow-typed files, install `babel-eslint` and add it to the config:
+
+```
+npm i -D babel-eslint


Use long-hand for documentation: npm install --save-dev babel-eslint

pvdlg · 2018-05-21T20:33:58Z

Hmmm getting an import/no-unresolved error for .ts files. But only if the extension is excluded in the import

Sounds like something to report to eslint-plugin-import project.

sindresorhus · 2018-05-21T20:36:21Z

I think you need to set the import/parser option too.

sindresorhus · 2018-05-21T20:36:53Z

readme.md

+}
+```
+
+### TypeScript


Place TypeScript first since it's more popular.

jorgegonzalez · 2018-05-21T20:44:02Z

Oh cool, setting import/parsers seems like a better solution than what I had done previously.

pvdlg · 2018-05-21T20:44:40Z

config/plugins.js

@@ -25,7 +25,17 @@ module.exports = {
 		'import/core-modules': [
 			'electron',
 			'atom'
-		]
+		],
+		'import/resolver': {


Shouldn't it be as follow?

import/parsers: { typescript-eslint-parser: [ .ts, .tsx ] }

Also should we add that only for ts/tsx files similarly to parser = 'typescript-eslint-parser'?

Actually still need this or the import/unresolved rule throws.

So it should be done dynamically in the index.js only for ts/tsx files I imagine

I don't think it's an issue unless someone is intentionally mixing .js and .ts files in a directory.

Could you move the import/parsers and import/resolver in index.js so it will be enabled only for ts and tsx files? This way that would avoid creating additional load and add a dependency to typescript-eslint-parser for non TS file.

if (paths.filter(filePath => isTSFile(path.extname(filePath))).length > 0) { opts.parser = 'typescript-eslint-parser'; opts['import/resolver'].node.extensions = ['.js', '.jsx', '.ts', 'tsx']; opts['import/parsers'].['typescript-eslint-parser'] = ['.ts', 'tsx']; }

pvdlg · 2018-05-21T21:51:31Z

index.js

+
+		if (paths.filter(filePath => isTSFile(path.extname(filePath))).length > 0) {
+			opts.parser = 'typescript-eslint-parser';
+			opts['settings'] = {


I don't think this is correct as with the current code the entire settings value will be overridden.
We want to add/merge only the last leaf (extensions and typescript-eslint-parser).
So if the user config already contains something like:

{ settings: { 'import/resolver': { node: { someConf: 'someValue' } } } }

We want to end up with:

{ settings: { 'import/resolver': { node: { extensions: ['.js', '.jsx', '.ts', '.tsx'], someConf: 'someValue' } } } }

pvdlg · 2018-05-21T21:52:00Z

index.js

+
+		if (paths.filter(filePath => isTSFile(path.extname(filePath))).length > 0) {
+			opts.parser = 'typescript-eslint-parser';
+			opts['settings'] = {


opts['settings'] should be opts.settings for consistency.

pvdlg · 2018-05-21T22:09:19Z

index.js

@@ -102,6 +102,20 @@ module.exports.lintFiles = (patterns, opts) => {
 			});
 		}

+		const isTSFile = ext => ext === '.ts' || ext === '.tsx';
+
+		if (paths.filter(filePath => isTSFile(path.extname(filePath))).length > 0) {


Instead of filter + .length > 0 you'll get better performances with find as it will stop on the first item found . rather than going through the whole list:

if (paths.find(filePath => isTSFile(path.extname(filePath))))

jorgegonzalez · 2018-05-21T23:07:58Z

index.js

+		if (paths.find(filePath => isTSFile(path.extname(filePath)))) {
+			opts.parser = 'typescript-eslint-parser';
+
+			const extensions =


There is probably a better way to do this

You should use mergeWith like in the rest of the codebase: https://github.com/xojs/xo/blob/master/lib/options-manager.js#L168

pvdlg · 2018-05-22T00:08:31Z

config/plugins.js

+		'no-use-extend-native',
+		'node',
+		'promise',
+		'typescript',


This is different than the scope of this PR. If we start using eslint-plugin-typescript this should be done in a different PR, that might potentially be released later when the plugin is stable.

I disagree. It's pretty usable right now, just missing a lot of rules. And I think enabling additional TypeScript-specific rules is definitely in the scope of a PR titled "Add support for Flow and TypeScript".

The first sentence of the readme is This is still in the very early stages, so please be patient.
So it's pretty clear it's not production ready and therefore not ready to added to XO.
That said XO is pretty configurable so nothing prevent you to configure the plugin plugin manually or to create a shareable config that includes it.

"Early stages" does not imply "unstable" or "not production ready". Recent npm stats indicate that users have confidence in its current usability.

The problem is that we'll be blamed if something doesn't work. We will also constantly have to bump major to be able to upgrade as they seem to do breaking changes often. I think it's wise to require it to be added as a dependency for now and we can open an issue about including it by default in the future.

jorgegonzalez · 2018-05-22T18:25:18Z

@pvdlg I really appreciate your help and all the code reviews!

sindresorhus · 2018-05-30T09:11:57Z

@jorgegonzalez I've made https://github.com/xojs/eslint-config-xo-typescript now. Can you mention it in the readme?

jorgegonzalez · 2018-05-31T21:10:31Z

@sindresorhus Do you also want me to mention xojs/eslint-config-xo-flow?

sindresorhus · 2018-05-31T21:25:07Z

Yes

jorgegonzalez · 2018-06-12T14:51:37Z

Does anything else need to be done to merge this?

pvdlg · 2018-06-12T15:12:11Z

It seems we decided to go with the option of using eslint-config-xo-typescript and eslint-config-xo-flow. So the code changes in index.js should be removed.
The only code change to keep is to add .ts and .tsx to DEFAULT_EXTENSION.

The doc should also be modified to simply:

provide a link to eslint-config-xo-flow#use-with-xo for Flow
provide a link to eslint-config-xo-typescript#use-with-xo for Typescript

jorgegonzalez and others added 4 commits May 17, 2018 19:25

Add support for Flow and TypeScript

7c1afe1

Add Flow and TypeScript usage documentation

8cebc86

Warn against setting both typescript and flow

a528d75

Typo

7ea0888

jorgegonzalez added 3 commits May 18, 2018 09:17

Make typescript usage simpler

1bc923d

Merge

d0c24d3

Update Flow and TypeScript documentation

e6ce8f5

jorgegonzalez added 2 commits May 18, 2018 09:33

Update readme.md

b995d75

More headers

cdf9b10

pvdlg reviewed May 18, 2018

View reviewed changes

Use path.extname instead

8b852d3

jorgegonzalez mentioned this pull request May 20, 2018

Extensions/Parser not recognized in options xojs/vscode-linter-xo#43

Closed

sindresorhus reviewed May 21, 2018

View reviewed changes

Update readme.md

3fc78e2

sindresorhus reviewed May 21, 2018

View reviewed changes

jorgegonzalez added 2 commits May 21, 2018 16:38

Fix import/no-unresolved issue for TypeScript

8592265

Update readme.md

9081ec2

pvdlg reviewed May 21, 2018

View reviewed changes

jorgegonzalez added 2 commits May 21, 2018 16:45

Set typescript-eslint-parser in config instead

c9d3c78

Update plugins.js

9419110

jorgegonzalez and others added 2 commits May 21, 2018 17:41

Only modify import/parser and import/resolver for TypeScript files

2302302

Comma

35293f8

pvdlg reviewed May 21, 2018

View reviewed changes

jorgegonzalez and others added 4 commits May 21, 2018 18:17

Merge import/resolver extensions instead of overriding object

ab3a91c

Add eslint-plugin-typescript

0c45dea

Must check if nested properties are valid

815e9b4

Fix bug

8de9f33

jorgegonzalez commented May 21, 2018

View reviewed changes

pvdlg reviewed May 22, 2018

View reviewed changes

jorgegonzalez added 2 commits May 21, 2018 20:33

Use lodash.mergewith

cab6dfe

Whitespace

f036f9e

jorgegonzalez mentioned this pull request May 22, 2018

Add support for Flow #276

Closed

jorgegonzalez added 3 commits May 22, 2018 11:57

Update plugins.js

930320c

Update plugins.js

473ea9b

Update package.json

ac4e9ee

jorgegonzalez changed the title ~~Add support for Flow and TypeScript~~ Add support for TypeScript and document usage with Flow May 25, 2018

Update readme.md

8fb613e

jorgegonzalez added 2 commits June 12, 2018 11:22

Update index.js

bb2bed1

Update readme.md

165ef0f

pvdlg approved these changes Jun 12, 2018

View reviewed changes

Update readme.md

8e8dd03

sindresorhus merged commit 957b0d9 into xojs:master Jun 24, 2018

Add support for TypeScript and document usage with Flow #326

Add support for TypeScript and document usage with Flow #326

Conversation

jorgegonzalez commented May 17, 2018 • edited

pvdlg commented May 18, 2018

jorgegonzalez commented May 18, 2018

jorgegonzalez commented May 18, 2018

Choose a reason for hiding this comment

pvdlg commented May 18, 2018

jorgegonzalez commented May 18, 2018 • edited

pvdlg commented May 18, 2018

jorgegonzalez commented May 21, 2018 • edited

Choose a reason for hiding this comment

pvdlg commented May 21, 2018

sindresorhus commented May 21, 2018

Choose a reason for hiding this comment

jorgegonzalez commented May 21, 2018

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

pvdlg May 21, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvdlg May 21, 2018 • edited

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

jorgegonzalez May 22, 2018 • edited

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

jorgegonzalez commented May 22, 2018

sindresorhus commented May 30, 2018

jorgegonzalez commented May 31, 2018

sindresorhus commented May 31, 2018

jorgegonzalez commented Jun 12, 2018

pvdlg commented Jun 12, 2018

jorgegonzalez commented May 17, 2018 •

edited

jorgegonzalez commented May 18, 2018 •

edited

jorgegonzalez commented May 21, 2018 •

edited

pvdlg May 21, 2018 •

edited

pvdlg May 21, 2018 •

edited

jorgegonzalez May 22, 2018 •

edited