Add TypeScript support #426

pvdlg · 2020-02-17T22:37:41Z

Fix #373
Fix xojs/eslint-config-xo-typescript#15

Xo will now automatically lint .ts and .tsx with the config from eslint-config-xo-typescript. No configuration needed!

IssueHunt Summary

Referenced issues

This pull request has been submitted to:

#373: Built-in support for TypeScript

IssueHunt has been backed by the following sponsors. Become a sponsor

sindresorhus · 2020-02-18T06:34:16Z

Can you make it handle .d.ts too?

Per issue description:

Should lint foo.d.ts (TS definition files) in non-TS projects automatically.

I don't have that many actual TypeScript projects, but I have hundreds of JS packages with a *.d.ts file and it would be nice to be able to enforce best practices in those.

pvdlg · 2020-02-18T06:49:34Z

Can you make it handle .d.ts too?

It should lint .d.ts files. index.d.ts is considered to have the extension .ts so it would be included.
Did you encountered an issue linting a .d.ts file?

sindresorhus · 2020-02-18T07:09:30Z

Good point. I was out on my phone and didn't check yet.

sindresorhus · 2020-02-18T07:11:45Z

There's a problem with npm link'ing XO. I tried npm link in XO and then npm link xo in, for example, https://github.com/sindresorhus/make-dir, and I'm getting:

Error: Failed to load config "xo-typescript" to extend from.
Referenced from: BaseConfig
    at configMissingError (/Users/sindresorhus/dev/oss/xo/node_modules/eslint/lib/cli-engine/config-array-factory.js:265:9)
    at ConfigArrayFactory._loadExtendedShareableConfig (/Users/sindresorhus/dev/oss/xo/node_modules/eslint/lib/cli-engine/config-array-factory.js:826:23)
    at ConfigArrayFactory._loadExtends (/Users/sindresorhus/dev/oss/xo/node_modules/eslint/lib/cli-engine/config-array-factory.js:731:25)
    at ConfigArrayFactory._normalizeObjectConfigDataBody (/Users/sindresorhus/dev/oss/xo/node_modules/eslint/lib/cli-engine/config-array-factory.js:660:25)
    at _normalizeObjectConfigDataBody.next (<anonymous>)
    at ConfigArrayFactory._normalizeObjectConfigData (/Users/sindresorhus/dev/oss/xo/node_modules/eslint/lib/cli-engine/config-array-factory.js:596:20)
    at _normalizeObjectConfigData.next (<anonymous>)
    at createConfigArray (/Users/sindresorhus/dev/oss/xo/node_modules/eslint/lib/cli-engine/config-array-factory.js:340:25)
    at ConfigArrayFactory.create (/Users/sindresorhus/dev/oss/xo/node_modules/eslint/lib/cli-engine/config-array-factory.js:395:16)
    at createBaseConfigArray (/Users/sindresorhus/dev/oss/xo/node_modules/eslint/lib/cli-engine/cascading-config-array-factory.js:86:48)

It works if I manually copy the XO directory into node_modules of make-dir.

sindresorhus · 2020-02-18T07:12:50Z

It works if I manually copy the XO directory into node_modules of make-dir.

However, then there are some parse errors:

~/dev/oss/make-dir master
❯ xo

  index.test-d.ts:2:16
  ✖  2:16  Parsing error: Unexpected token =

  index.d.ts:4:9
  ✖  4:9   Parsing error: Unexpected token namespace

  2 errors

sindresorhus · 2020-02-18T07:15:41Z

There's also a problem with reading tsconfig.json directly. tsconfig.json supports comments and it's very common to put it in there. I would look into how @typescript-eslint/eslint-plugin reads the tsconfig.

Here's the error:

~/dev/oss/got
❯ xo
JSONError: JSON Error in /Users/sindresorhus/dev/oss/got/tsconfig.json:

  3 | 	"compilerOptions": {
  4 | 		"outDir": "dist",
> 5 | 		"target": "es2018", // Node.js 10
    | 		                    ^
  6 | 		"lib": [
  7 | 			"es2018"
  8 | 		]

pvdlg · 2020-02-18T07:18:43Z

The loading is probably related to how ESlint resolve config/plugin names. I'll look into it to see if something can done (maybe passing absolute path to ESlint?)

I'll check the parsing errors and the comment in tsconfig.json tomorrow.

sindresorhus · 2020-02-18T07:19:48Z

Non-TS projects with a d.ts file don't usually have a tsconfig.json file, but TS is way too loose by default. We should include some strict defaults for such cases, by using https://github.com/sindresorhus/tsconfig/blob/master/tsconfig.json

pvdlg · 2020-02-18T07:26:50Z

Non-TS projects with a d.ts file don't usually have a tsconfig.json file, but TS is way too loose by default. We should include some strict defaults for such cases, by using https://github.com/sindresorhus/tsconfig/blob/master/tsconfig.json

I'm not sure it's possible. We look at the tsconfig.json only to figure out the path and pass it to parserOptions.project but it's the parser that loads it. It doesn't seems to be a way to pass the config to the parser programatically.

sindresorhus · 2020-02-18T07:47:39Z

I'm not sure it's possible. We look at the tsconfig.json only to figure out the path and pass it to parserOptions.project but it's the parser that loads it. It doesn't seems to be a way to pass the config to the parser programatically.

typescript-eslint/typescript-eslint#1613

sindresorhus · 2020-02-18T06:36:09Z

lib/options-manager.js

@@ -47,6 +55,8 @@ const mergeFn = (previousValue, value) => {
 	}
 };

+const isTypescript = file => TYPESCRIPT_EXTENSION.includes(path.extname(file).replace('.', ''));


Suggested change

const isTypescript = file => TYPESCRIPT_EXTENSION.includes(path.extname(file).replace('.', ''));

const isTypescript = file => TYPESCRIPT_EXTENSION.includes(path.extname(file).slice(1));

Since you know what extname will always return a string with . in front or an empty string. It should be safer than just replacing the first dot.

readme.md

pvdlg · 2020-02-18T08:01:21Z

I can't reproduce the issue you are having on https://github.com/sindresorhus/make-dir

I cloned the repo, then npm install -D xojs/xo#typesscript. I also had to add typescript to the project with npm install typescript -D which is a required dependency of the parser it seems.
Finally I had to add a tsconfig.json with:

{
  "include": ["**/*.ts"]
}

Then running xo:

~/g/make-dir (master ✚1…1) ❯ xo                                                                                                                 264ms

  index.test-d.ts:11:38
  ✖  11:38  Use octal literals instead of parseInt().  prefer-numeric-literals
  ✖  20:42  Use octal literals instead of parseInt().  prefer-numeric-literals

  2 errors

It seems that a tsconfig.json is required by @typescript-eslint/parser. From the doc:

This setting is required if you want to use rules which require type information

Maybe we can try to figure out which rules require parserOptions.project and disable them if no tsconfig.json is found. But that would be a pain to maintain as every time we add a rule to eslint-config-xo-typescript we would have to disable it in XO when no tsconfig.json is found.

Another solution is to enable the Typescript rules only when a tsconfig.json is found even if the file is a .ts. Which would require projects with only a .d.ts file to add a tsconfig.json.

sindresorhus · 2020-02-18T08:14:13Z

Maybe we can try to figure out which rules require parserOptions.project and disable them if no tsconfig.json is found. But that would be a pain to maintain as every time we add a rule to eslint-config-xo-typescript we would have to disable it in XO when no tsconfig.json is found.

Yeah, I don't think that's a realistic solution.

Another solution is to enable the Typescript rules only when a tsconfig.json is found even if the file is a .ts. Which would require projects with only a .d.ts file to add a tsconfig.json.

That would severely limit the usefulness of this. There are much more projects with .d.ts than there are pure TS projects and I don't want to have to add a tsconfig to pure JS projects.

sindresorhus · 2020-02-18T08:15:21Z

Hmm, an idea. We could just write out a tsconfig file to a temp directory and point parserOptions.project to that.

pvdlg · 2020-02-18T16:17:33Z

For info typescript-eslint/typescript-eslint#406 which is essentially the same as the one you opened hasn't been answered.

Also see typescript-eslint/typescript-eslint#689. It seems the tsconfig.json file is used at a minimum to read the include property to figure out all the files that are part of the project and compile them.

To be honest I though a tsconfig.json was mandatory for every TypeScript project and I didn't think about JS project have a type definition file.

If we add a default tsconfig.json I'm not sure if we should set the include with just the file we are currently linting or with ["**/*.ts",**/*.tsx"]. In the first case we wouldn't lint much as we would loose the ability to use the type definition from other files. In the other case we might create unintended behaviors on some projects.

sindresorhus · 2020-02-18T16:21:52Z

If we add a default tsconfig.json I'm not sure if we should set the include with just the file we are currently linting or with ["/*.ts",/*.tsx"]. In the first case we wouldn't lint much as we would loose the ability to use the type definition from other files. In the other case we might create unintended behaviors on some projects.

We should only do it for .d.ts files. Actual TypeScript projects are expected to have their own tsconfig file.

sindresorhus · 2020-02-18T16:23:46Z

Hmm, maybe we could fix xojs/eslint-config-xo-typescript#15 at the same time (It's a huge pain-point). Often the tsconfig file contains more files than you actually want to lint. We could read the tsconfig file and switch out includes with only the files being linted.

// @fregante

pvdlg · 2020-02-18T19:08:10Z

After doing a lot of tests, we can create a temporary one and pass it to parserOptions.projet. However the temporary tsconfig.json has to contains absolute paths in the include property.

This will not work:

{
  "include": ["**/*.ts"]
}

This will work:

{
  "include": ["/users/pvdlg/project/**/*.ts", "/users/pvdlg/project/**/*.tsx"]
}

So we could do:

if we don't find a tsconfig.json => create a temp one with "include": ["${options.cwd}/**/*.ts", "${options.cwd}/**/*.tsx"]
if we find a tsconfig.json => create a temp one with "include": ["${options.cwd}/**/*.ts", "${options.cwd}/**/*.tsx"] that extends the file we've found

I think it should solve both xojs/eslint-config-xo-typescript#15 and projects without a tsconfig.json.

That seems like a clunky workaround, but it's the only solution I've found.

sindresorhus · 2020-02-18T19:39:52Z

if we don't find a tsconfig.json => create a temp one with "include": ["${options.cwd}//*.ts", "${options.cwd}//*.tsx"]

I'm not sure how @typescript-eslint/parser works under the hood with the include option, but just globbing everything might be slow. Wouldn't it be better to pass in the .ts files XO has already found?

Also, here we should inject some sensible defaults: https://github.com/sindresorhus/tsconfig/blob/9c49b210a7e7dee5a0feeb8433c8cc165f6319bb/tsconfig.json#L15-L19

That seems like a clunky workaround, but it's the only solution I've found.

I'm totally fine with it being clunky if it fixes the problem.

pvdlg · 2020-02-24T03:30:59Z

@sindresorhus I updated the PR per our discussion.
This should solve the case of projects without a tsconfig.json and xojs/eslint-config-xo-typescript#15.

Note that in the case of project without a tsconfig.json, when linting a subset of files @typescript-eslint/eslint-plugin will have no way to know about other files in the project and that might affect how the rules behave.
For example if you have a project with files src/a.ts and src/b.ts and run xo src/a.ts @typescript-eslint/eslint-plugin will execute its rule without knowledge of b.ts. Not sure if that's really an issue though. Plus there is nothing we can do other than globing the whole project...

This problem will not happen if you have a tsconfig.json.

sindresorhus · 2020-02-24T09:57:12Z

Can you also ensure the generated tsconfig is strict by adding this config to it?

"target": "es2018", // Node.js 10
"newLine": "lf",
"strict": true,
"noImplicitReturns": true,
"noUnusedLocals": true,
"noUnusedParameters": true,
"noFallthroughCasesInSwitch": true,

sindresorhus · 2020-02-24T09:58:02Z

Can you mention in the readme that it works for d.ts files in JS projects without a tsconfig file? I don't think that would be obvious to the user.

sindresorhus · 2020-02-24T10:50:22Z

I've been testing this on a bunch of projects and it seems to work great. 👌

lib/options-manager.js

pvdlg · 2020-02-24T18:11:10Z

I made the requested changes.

I added the default config only when no tsconfig.json is found.
I don't think it's a good idea to add those when the project has a tsconfig.json because it would result in the linter using different options than the the compiler. Also there is a lot of project that doesn't set options like strict, noImplicitReturns and use the default on purpose. For example when transitioning from JS to TS. What do you think?

sindresorhus · 2020-02-24T18:38:43Z

Oh yeah. Definitely. I meant when there’s no tsconfjg file.

sindresorhus · 2020-02-24T18:57:50Z

Manually tested it again and seems to work great.

sindresorhus · 2020-02-24T18:59:01Z

Woooot! Really awesome that you decided to implement this. 🎉✨

Should I do a new release?

bmealhouse · 2020-02-24T19:03:11Z

Very excited for these changes. Thanks so much @pvdlg!

pvdlg · 2020-02-24T19:07:49Z

Should I do a new release?

Yes that would be awesome!

sindresorhus · 2020-02-24T19:32:45Z

It's out: https://github.com/xojs/xo/releases/tag/v0.27.0

pvdlg requested a review from sindresorhus February 17, 2020 22:37

sindresorhus mentioned this pull request Feb 18, 2020

Allow setting tsconfig config directly in the parser typescript-eslint/typescript-eslint#1613

Open

sindresorhus requested changes Feb 18, 2020

View reviewed changes

sindresorhus reviewed Feb 24, 2020

View reviewed changes

lib/options-manager.js Outdated Show resolved Hide resolved

pvdlg and others added 4 commits February 24, 2020 11:41

Add TypeScript support

bf655dc

Update package.json

4d61294

Update readme.md

8bc36cb

Don't fail is tsconfig.json contains comments

c90b5f2

sindresorhus merged commit b0dfcbd into master Feb 24, 2020

sindresorhus deleted the typescript branch February 24, 2020 18:58

This was referenced Feb 24, 2020

Various fixes to TypeScript integration #431

Merged

XO should not lint type definition files in a non-TS project #348

Closed

This was referenced Mar 19, 2020

[Snyk] Upgrade xo from 0.27.0 to 0.27.1 salty-pig/swapi-node#69

Merged

[Snyk] Upgrade xo from 0.25.3 to 0.27.2 nodeshift-starters/nodejs-health-check#171

Closed

[Snyk] Upgrade xo from 0.25.3 to 0.27.2 nodeshift-starters/nodejs-rest-http#165

Closed

fregante mentioned this pull request Apr 5, 2020

Error: Failed to load config "xo-typescript" to extend from. #454

Closed

snyk-bot mentioned this pull request Apr 16, 2020

[Snyk] Upgrade xo from 0.25.3 to 0.28.0 nodeshift-starters/nodejs-cache#62

Closed

snyk-bot mentioned this pull request May 6, 2020

[Snyk] Upgrade xo from 0.26.1 to 0.30.0 averissimo/write2sheet#29

Closed

mfranzke mentioned this pull request Nov 18, 2023

List typescript extensions as well for extensions config #739

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add TypeScript support #426

Add TypeScript support #426

pvdlg commented Feb 17, 2020 •

edited

sindresorhus commented Feb 18, 2020

pvdlg commented Feb 18, 2020

sindresorhus commented Feb 18, 2020

sindresorhus commented Feb 18, 2020

sindresorhus commented Feb 18, 2020

sindresorhus commented Feb 18, 2020

pvdlg commented Feb 18, 2020

sindresorhus commented Feb 18, 2020

pvdlg commented Feb 18, 2020

sindresorhus commented Feb 18, 2020

sindresorhus Feb 18, 2020

pvdlg commented Feb 18, 2020

sindresorhus commented Feb 18, 2020

sindresorhus commented Feb 18, 2020

pvdlg commented Feb 18, 2020

sindresorhus commented Feb 18, 2020

sindresorhus commented Feb 18, 2020

pvdlg commented Feb 18, 2020

sindresorhus commented Feb 18, 2020

pvdlg commented Feb 24, 2020

sindresorhus commented Feb 24, 2020

sindresorhus commented Feb 24, 2020

sindresorhus commented Feb 24, 2020

pvdlg commented Feb 24, 2020

sindresorhus commented Feb 24, 2020

sindresorhus commented Feb 24, 2020

sindresorhus commented Feb 24, 2020

bmealhouse commented Feb 24, 2020

pvdlg commented Feb 24, 2020

sindresorhus commented Feb 24, 2020

	const isTypescript = file => TYPESCRIPT_EXTENSION.includes(path.extname(file).replace('.', ''));
	const isTypescript = file => TYPESCRIPT_EXTENSION.includes(path.extname(file).slice(1));

Add TypeScript support #426

Add TypeScript support #426

Conversation

pvdlg commented Feb 17, 2020 • edited

Referenced issues

sindresorhus commented Feb 18, 2020

pvdlg commented Feb 18, 2020

sindresorhus commented Feb 18, 2020

sindresorhus commented Feb 18, 2020

sindresorhus commented Feb 18, 2020

sindresorhus commented Feb 18, 2020

pvdlg commented Feb 18, 2020

sindresorhus commented Feb 18, 2020

pvdlg commented Feb 18, 2020

sindresorhus commented Feb 18, 2020

sindresorhus Feb 18, 2020

Choose a reason for hiding this comment

pvdlg commented Feb 18, 2020

sindresorhus commented Feb 18, 2020

sindresorhus commented Feb 18, 2020

pvdlg commented Feb 18, 2020

sindresorhus commented Feb 18, 2020

sindresorhus commented Feb 18, 2020

pvdlg commented Feb 18, 2020

sindresorhus commented Feb 18, 2020

pvdlg commented Feb 24, 2020

sindresorhus commented Feb 24, 2020

sindresorhus commented Feb 24, 2020

sindresorhus commented Feb 24, 2020

pvdlg commented Feb 24, 2020

sindresorhus commented Feb 24, 2020

sindresorhus commented Feb 24, 2020

sindresorhus commented Feb 24, 2020

bmealhouse commented Feb 24, 2020

pvdlg commented Feb 24, 2020

sindresorhus commented Feb 24, 2020

pvdlg commented Feb 17, 2020 •

edited