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

Migrate the repo to ESLint #30553

Closed
DanielRosenwasser opened this issue Mar 22, 2019 · 10 comments · Fixed by #31777
Closed

Migrate the repo to ESLint #30553

DanielRosenwasser opened this issue Mar 22, 2019 · 10 comments · Fixed by #31777
Assignees
Labels
Infrastructure Issue relates to TypeScript team infrastructure

Comments

@DanielRosenwasser
Copy link
Member

Since we've been working on bringing linting parity with TSLint to ESLint, we need to start using it ourselves. This issue tracks exactly that.

@Ethan-Arrowood
Copy link

Hi! I have a new guide on setting up ESLint with TypeScript: https://blog.matterhorn.dev/posts/learn-typescript-linting-part-1/

I'd be happy to try and set it up for y'all or someone else can do it themselves. Let me know!

@jessetrinity
Copy link
Contributor

@Ethan-Arrowood this is a really helpful guide, thank you!

@Ethan-Arrowood
Copy link

You're welcome @jessetrinity

If anything is unclear please let me know. I'm happy to help

@Ethan-Arrowood
Copy link

I have opened a PR to add my resource to the tutorial section of the handbook: microsoft/TypeScript-Handbook#1046

If my work is better linked somewhere else please let me know; I'd love for my work to help as many TypeScript developers as possible

@David-Else
Copy link

@Ethan-Arrowood Nice guide! Would be great to get it actually in the handbook, If you supplied the .md files rather than a link I bet the powers that be would be even more likely to include it :)

Have you checked out the plugin:@typescript-eslint/eslint-recommended option? The docs don't mention it much, but I think your guide would benefit from it:

The eslint-recommended ruleset is meant to be used after extending eslint:recommended. It disables rules that are already checked by the Typescript compiler and enables rules that promote using more the more modern constructs Typescript allows for.

https://github.com/typescript-eslint/typescript-eslint/tree/b5e95edbb811ba239d2856e9caee21e802fbd8fb/packages/eslint-plugin/src/configs

It works well after other JS rulesets with things that should be overridden. Here is my current config that seems to be working well:

{
  "extends": [
    "./node_modules/eslint-config-airbnb-base/rules/best-practices.js",
    "./node_modules/eslint-config-airbnb-base/rules/errors.js",
    "./node_modules/eslint-config-airbnb-base/rules/node.js",
    "./node_modules/eslint-config-airbnb-base/rules/style.js",
    "./node_modules/eslint-config-airbnb-base/rules/variables.js",
    "./node_modules/eslint-config-airbnb-base/rules/es6.js",
    "plugin:@typescript-eslint/eslint-recommended", <<<<<<<<<<<<<<<<<<<<<<<<<<<<<
    "plugin:@typescript-eslint/recommended",
    "prettier",
    "prettier/@typescript-eslint"
  ],
  "parser": "@typescript-eslint/parser",
  "parserOptions": {
    "project": "./tsconfig.json" <<<<<<<<<<<<<<<<<<<<<<<<<<<<<
  },
  "plugins": ["@typescript-eslint"],
  "rules": {
    "lines-between-class-members": "off",
    "no-empty-function": "off",
    "no-useless-constructor": "off",
    "@typescript-eslint/member-ordering": "warn",
    "@typescript-eslint/no-unused-vars": "off",
    "@typescript-eslint/no-useless-constructor": "warn",
    "@typescript-eslint/no-parameter-properties": "off",
    "@typescript-eslint/explicit-function-return-type": [
      "warn",
      {
        "allowExpressions": true
      }
    ]
  },
  "env": {
    "browser": true,
    "node": true,
    "mocha": true
  },
  "globals": {
    "assert": true
  }
}

I noticed in your guide you didn't mention "project": "./tsconfig.json" which is needed to allow rules to have access to the types:

https://github.com/typescript-eslint/typescript-eslint/tree/master/packages/eslint-plugin

If you want to use rules which require type information, you will need to specify a path to your tsconfig.json file in the "project" property of "parserOptions".

If you look at the main grid of rules the ones that require types are marked with a little cloud? icon.

Sorry if I am mentioning something I missed in your guide, I only had time to read them quickly. Have you told the typescript-eslint people about it?

@Ethan-Arrowood
Copy link

Hi @DanielRosenwasser is the plugin:@typescript-eslint/eslint-recommended different from plugin:@typescript-eslint/recommended? I'll have to look into that more.

I definitely did miss the "project": "./tsconfig.json" part. I'll make note of this and work on adding it to the guide asap!

Thank you for the insight 😄

My only issue with submitting my whole post to the handbook is that the guide is fairly long and I don't know if users would want to read so much stuff. Additionally, as things change or need updating I'd like to only maintain it in one place (i.e. my blog). Just like with the changes you just recommended it would be challenging to have to maintain updates across multiple different sources.

@KagamiChan
Copy link
Member

Hi @Ethan-Arrowood and @David-Else
This issue is for tracking the migration of TypeScript repo, could we have another thread for the guide? So that subscribers like me could have less distractions, Thanks!

Couto added a commit to yldio/conference-template that referenced this issue Aug 28, 2019
- Remove tslint and its configurations
- Add eslint and set default configurations

Palantir [announced][1] that it will deprecate TSLint in favour of
typescript-eslint.

The TypeScript team [announced][2] that themselves will start using
typescript-eslint instead of tslint.

[1]: https://medium.com/palantir/tslint-in-2019-1a144c2317a9
[2]: microsoft/TypeScript#30553
Couto added a commit to yldio/conference-template that referenced this issue Aug 28, 2019
- Remove tslint and its configurations
- Add eslint and set default configurations

Palantir [announced][1] that it will deprecate TSLint in favour of
typescript-eslint.

The TypeScript team [announced][2] that themselves will start using
typescript-eslint instead of tslint.

[1]: https://medium.com/palantir/tslint-in-2019-1a144c2317a9
[2]: microsoft/TypeScript#30553
@kdawgwilk
Copy link

Just out of curiosity, was this change benchmarked? I know I recently attempted to migrate from TSLint to ESLint and found it was significantly slower on a fairly large codebase (~40k LOC)

@uniqueiniquity
Copy link
Contributor

Yes - we found comparable lint times before and after the migration (generally, ~35 seconds before, ~45 seconds after). However, when using the --cache flag for ESLint, we saw our lint times go down to ~19 seconds or so (with a lot of varying based on the changes made, of course). We've also been in contact with the maintainers of the TypeScript-ESLint project to help drive perf improvements.

@khaledosman
Copy link

@kdawgwilk I had a similar issue, but even with a small codebase.

if you're using webpack https://github.com/TypeStrong/fork-ts-checker-webpack-plugin#readme can save you lots of time aswell as these webpack tips.

Also make sure you exclude node_modules as well as build and non-typescript files from your tsconfig.json, webpack.json loaders and eslint setup via an .eslintignore file

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Infrastructure Issue relates to TypeScript team infrastructure
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants