Skip to content
This repository has been archived by the owner on Nov 8, 2021. It is now read-only.

Integrate linting with official ESLint TypeScript plugin #18

Closed
FerrielMelarpis opened this issue May 16, 2019 · 18 comments
Closed

Integrate linting with official ESLint TypeScript plugin #18

FerrielMelarpis opened this issue May 16, 2019 · 18 comments

Comments

@FerrielMelarpis
Copy link

FerrielMelarpis commented May 16, 2019

When you opt-in to eslint feature of quasar-cli, it will inject the eslint-loader to the webpack config. Problem is even if the <script> uses lang="ts" on a SFC, it will still run against it.

// quasar.conf.js
extendWebpack(cfg) {
  cfg.module.rules.push({
    enforce: 'pre',
    test: /\.(js|vue)$/,
    loader: 'eslint-loader',
    exclude: /node_modules/,
  });
},

Sample error when using vue-property-decorator

  33:11  error  Parsing error: Unexpected token

  17 | export default class UserList extends Vue {
  18 |   @Prop({ type: Array, default: () => [] })
> 19 |   readonly users: any[];
     |            ^

Cannot recognize readonly as a keyword since it is only recognized by typescript.

Not sure if the best option is to use tslint-loader. investigate typescript-eslint

@GordonBlahut
Copy link

TSLint is going to be deprecated so it probably doesn't make sense to use tslint-loader.

I'm using eslint (and therefore eslint-loader) with the @typescript-eslint/eslint-plugin package instead.

Once that plugin is configured, eslint-loader's text regex can be updated to include typescript files as well:

.test(/\.(vue|(j|t)sx?)$/)

It's pretty slow in my experience, so I only use it for quasar build and not quasar dev.

@FerrielMelarpis
Copy link
Author

@GordonBlahut Thanks for suggesting @typescript-eslint/eslint-plugin.
Are you able to set up quasar with that plugin?
May I know the steps on how you setup quasar + typescript + eslint + typescript-eslint?
Thanks

@IlCallo
Copy link
Member

IlCallo commented Jul 8, 2019

Noticed the same problem, resorted to exclude .vue files from ESLint for now.

I'll leave some link here to help setup of ESLint with TS support, which is the new preferred way, TSLint is going to be deprecated.

https://github.com/typescript-eslint/typescript-eslint
microsoft/TypeScript#30553
https://blog.matterhorn.dev/posts/learn-typescript-linting-part-1/

@IlCallo
Copy link
Member

IlCallo commented Jul 8, 2019

@FerrielMelarpis can you please rename the issue to something like "Integrate linting with official ESLint TypeScript plugin"?

@FerrielMelarpis FerrielMelarpis changed the title Replace eslint-loader with tslint-loader Integrate linting with official ESLint TypeScript plugin Jul 9, 2019
@FerrielMelarpis
Copy link
Author

Thanks @IlCallo
I was actually able to extend the webpack config of my quasar project and used typescript-eslint plugin and those links were the ones I used as well.
I can maybe put up a PR for it.

@IlCallo
Copy link
Member

IlCallo commented Jul 9, 2019

That would be a good thing :)

@outofmemoryagain
Copy link
Collaborator

outofmemoryagain commented Jul 9, 2019

Awesome, glad to see some progress on it. I can take a look at any PRs as needed. I'll keep an eye on this issue and it's progress.

@IlCallo
Copy link
Member

IlCallo commented Jul 9, 2019

Some discoveries I made, while exploring ESLint typescript plugin:

  • I manually checked TSLint and ESLint plugin: there is more or less parity about rules. Most rules were already borrowed from ESLint so no big deal, some TS-only ones are missing or has been renamed, but the most useful are there
  • Here are most of the needed steps to make it work
  • You must also take into account vue-eslint-parser in the mix, here is the relevant section on it's guide
  • It would be a good idea to also mention that with this change you don't need TSLint extension for VSCode anymore and can use ESLint version, as pointed out here. Obviously in our case we would need to add typescript into the managed languages

I'll add more while I proceed in the study

@IlCallo
Copy link
Member

IlCallo commented Jul 9, 2019

This is the .eslintrc.js I obtained until now, with comments to explain where I got all stuff.
Not perfectly working yet, seems like TS scripts into Vue SFC are not correctly linted.
Also, some rule groups may be left aside.
Linting via yarn lint also become really slow. Like, really slow.

Still, is something to work on.

Note that you should change lint script to include .ts files (I also included test folder for linting): "lint": "eslint --ext .js,.vue,.ts src test".

Interesting link: vuejs/eslint-plugin-vue#811

Current `.eslintrc.js` proposal for ESLint + TypeScript + Vue + Prettier
module.exports = {
  root: true,

  // Must use parserOptions instead of "parser" to allow vue-eslint-parser to keep working
  // See https://eslint.vuejs.org/user-guide/#how-to-use-custom-parser
  parserOptions: {
    parser: '@typescript-eslint/parser',
    sourceType: 'module',
    project: './tsconfig.json',
  },

  env: {
    browser: true,
  },

  // Order here IS important
  // See https://github.com/vuejs/eslint-plugin-vue/issues/811#issuecomment-467753982
  extends: [
    // Base ESLint recommended rules
    'eslint:recommended',
    // ESLint typescript rules
    // See https://github.com/typescript-eslint/typescript-eslint/tree/master/packages/eslint-plugin#usage
    'plugin:@typescript-eslint/eslint-recommended',
    'plugin:@typescript-eslint/recommended',
    // `plugin:vue/base` is the default, consider switching to `plugin:vue/strongly-recommended`
    //  or `plugin:vue/recommended` for stricter rules.
    // See https://github.com/vuejs/eslint-plugin-vue#priority-a-essential-error-prevention
    'plugin:vue/recommended',
    // Usage with Prettier, provided by 'eslint-config-prettier'.
    // https://github.com/typescript-eslint/typescript-eslint/tree/master/packages/eslint-plugin#usage-with-prettier
    'prettier',
    'prettier/@typescript-eslint',
    'prettier/vue',
  ],

  plugins: [
    // Required to apply rules which need type information
    '@typescript-eslint',
    // Required to lint *.vue files
    // See https://eslint.vuejs.org/user-guide/#why-doesn-t-it-work-on-vue-file
    'vue',
  ],

  globals: {
    ga: true, // Google Analytics
    cordova: true,
    __statics: true,
    process: true,
  },

  // add your custom rules here
  rules: {
    'prefer-promise-reject-errors': 'off',

    // allow console.log during development only
    'no-console': process.env.NODE_ENV === 'production' ? 'error' : 'off',
    // allow debugger during development only
    'no-debugger': process.env.NODE_ENV === 'production' ? 'error' : 'off',

    // Custom
    'vue/component-name-in-template-casing': ['error', 'kebab-case'],
    '@typescript-eslint/indent': ['error', 2],
    '@typescript-eslint/explicit-function-return-type': 'off',
  },
};
Important dependencies I currently have installed and their version (I may have missed some)
  • "@typescript-eslint/eslint-plugin": "1.11.0"
  • "@typescript-eslint/parser": "1.11.0"
  • "@vue/eslint-config-prettier": "4.0.1"
  • "eslint": "6.0.1"
  • "eslint-config-prettier": "6.0.0"
  • "eslint-loader": "2.2.1"
  • "eslint-plugin-vue": "5.2.3"

I tested all dependencies with ESLint 6 and they seem to work without problems, but not all of them already released ES6 battle-tested version (many already committed the change but did not published a release).

@IlCallo
Copy link
Member

IlCallo commented Jul 10, 2019

Other caveats:

  • Some Typescript ESLint plugin rules won't work for .vue files until next major version. This explains why I could not make them work as I wished, not because of Vetur as initially thought. Keeping TypeScript part into a separated file is currently the only way to correctly apply typescript linting
  • Most of the complains about rules I thought in these days are summarized in this issue which ask to remove too opinionated style rules from recommended set. It will probably happen into next major version.
  • There is a big time impact using ESLint with a particular import/namespace rule. It's a transitory problem and it's gonna be resolved when the corresponding PR is accepted.
  • Prettier has a somewhat important time impact on linting when used, but only when added as plugin.

In conclusion: next major (2.0.0) will probably be much more useful than current version.
I don't think that should be a blocker to the migration to ESLint, but we should consider list these caveats somewhere until then.

Integration guidelines?
Another point I think should be resolved, before proceeding further in the integration, is that once I managed to mix correctly every important ruleset into the ESLint pot, it appeared clear to me that most files (also configuration ones) should be updated to ES6 when applying the TS extension.

Most of those files are from other extensions (testing, etc).
@nothingismagick told me he wanted to manage Jest-TypeScript scenario into testing repo, not via TS extension.
Is this the preferred way? Have been all pros and cons of this choice considered? It seems a good idea to me, but I'm not that experienced, maybe it's better to list them somewhere (a new issue?)

@rstoenescu may I ask also your opinion on this?

@FerrielMelarpis
Copy link
Author

Linting via yarn lint also become really slow. Like, really slow.

This is due to limitations of ESlint. ESlint will generate the AST for every single file that it lints and then tsc would run type-check for each of the files. This is a bottleneck that typescript-eslint needs to pay for now unless ESlint provides a way to generate all linted files in one AST that can then be type-checked once. This flow would only happen when you specified the project in parserOptions. Removing it would make the compilation time a lot faster but I don't see why you would still use typescript-eslint if you are not using this feature anyway 🤷‍♂.

Prettier has a somewhat important time impact on linting when used, but only when added as plugin.

My workaround is to separate formatting and linting.
In my case, prettier-eslint works. Some other rules as well have an impact on the slowness of the compilation but that also depends on how big your codebase is.

I agree that we should be able to integrate a linting in this plugin but we should be keen to put the caveats on the documentation. FWIW, it would only be slow on initial compilation so a small price to pay for small to a medium codebase to keep them type-safe.

@outofmemoryagain
Copy link
Collaborator

Love to see the discussion and looking forward to making some progress on getting ESLint in place. Once some of the these pressing issues are completed I'm going to start a PR to add a Typescript section to the Quasar Docs, I would love your input on it when I do. If interested I will mention both of you so you can help review and contribute content.

@FerrielMelarpis
Copy link
Author

FerrielMelarpis commented Jul 11, 2019

@outofmemoryagain Glad to hear that 👍I'll spend some time to review that PR.
btw @IlCallo I'm not sure if you'd want to use @vue/prettier since it is designed for use of vue-cli.
IIRC, quasar-cli is a different beast. I use prettier/vue and added some rules as well using prettierrc

@IlCallo
Copy link
Member

IlCallo commented Jul 11, 2019

If interested I will mention both of you so you can help review and contribute content.

Yeah, ty! :)

btw @IlCallo I'm not sure if you'd want to use @vue/prettier since it is designed for use of vue-cli.
IIRC, quasar-cli is a different beast. I use prettier/vue and added some rules as well using prettierrc

That's the same thought I had, hence the // Where does this come from?? Why not `prettier/vue`? in the ESLint config I posted, and I didn't recall to having chosen it on my own.
So I checked into the quasar-starter-kit sources and I found that it's being added when you create a new project via Quasar CLI and select Prettier option.

I checked exactly what @vue/prettier does.

It just adds eslint:recommended, prettier (which disable recommended rules conflicting with Prettier) and prettier/vue rulesets, as well as adding prettier plugin (which generate some overhead, as I noted in my previous post).
It then adds prettier/prettier rule which refers to this known problem with prettier plugin.

We already added prettier ruleset and we are forced to require eslint-config-prettier to use prettier/@typescript-eslint, so we should probably drop @vue/prettier and use prettier/vue as suggested by @FerrielMelarpis

This would also allow to write them down with a sensible order.

Total extended rules, with ESLint + TypeScript + Vue + Prettier
  extends: [
    // Base ESLint recommended rules
    'eslint:recommended',
    // ESLint typescript rules
    // See https://github.com/typescript-eslint/typescript-eslint/tree/master/packages/eslint-plugin#usage
    'plugin:@typescript-eslint/eslint-recommended',
    'plugin:@typescript-eslint/recommended',
    // `plugin:vue/base` is the default, consider switching to `plugin:vue/strongly-recommended`
    //  or `plugin:vue/recommended` for stricter rules.
    // See https://github.com/vuejs/eslint-plugin-vue#priority-a-essential-error-prevention
    'plugin:vue/recommended',
    // Usage with Prettier, provided by 'eslint-config-prettier'.
    // https://github.com/typescript-eslint/typescript-eslint/tree/master/packages/eslint-plugin#usage-with-prettier
    'prettier',
    'prettier/@typescript-eslint',
    'prettier/vue',
  ],

I don't know how is the order "composable" at creation time tho

@bradzacher
Copy link

This is due to limitations of ESlint. ESlint will generate the AST for every single file that it lints and then tsc would run type-check for each of the files. This is a bottleneck that typescript-eslint needs to pay for now unless ESlint provides a way to generate all linted files in one AST that can then be type-checked once.

Nope! ESLint doesn't actually generate any ASTs or anything of the sort. In the default use case it defers this work to espree. In the typescript use case, you configure it to defer the work to @typescript-eslint/parser.

When you kick off a lint with parserOptions.project specified, we pass that to typescript and it does a complete parse and typecheck cycle up front on every single file specified within the tsconfig.json. Then when eslint invokes the parser in future, we already have the file parsed and typechecked.

It's been hard to dig into why people are seeing slowness (because most of our users have private codebases that they can't share...).
I believe some of the slowness is also due to users not specifying all their files in their tsconfig, which means eslint asks for ASTs for files that haven't been parsed up front, wasting some effort to construct a new program root.

Some of it is attributed to eslint-plugin-import accidentally invoking our parser the wrong way (fixed in the PR linked above).

@FerrielMelarpis
Copy link
Author

@bradzacher That's interesting to know. I can maybe investigate further and see if there's something I can do to make my builds faster without removing the benefit of linting with type-checking.

I believe some of the slowness is also due to users not specifying all their files in their tsconfig, which means eslint asks for ASTs for files that haven't been parsed up front, wasting some effort to construct a new program root.

What would specifying all their files in their tsconfig mean exactly? Not sure if this only pertains on includes, excludes options. Could you elaborate further? Sorry, I'm pretty new to the @typescript-eslint tooling 😄

@bradzacher
Copy link

Yup - within the tsconfig, you provide the include/exclude OR you provide files.
The typescript compiler loads every single file specified and treats it as the root of a dependency graph - i.e. it parses that file and detects its dependencies, then parses those files, etc until it has no more files to parse in the dependency graph.

Because you provide this up front, the typescript compiler treats all of these files as belonging to a single project, meaning it doesn't double parse any files.

(note that i'm not 100% on this next bit btw - I haven't dug into the parser enough to understand exactly)
If one file is not included in this giant parse cycle (say you had include:['src/*.ts'], but eslint wants to lint test/test.ts), then I believe typescript will treat this file as a brand new project, meaning it parses that file and all its dependencies separate from the original project (thus duplicating work and slowing the parse down).

@outofmemoryagain
Copy link
Collaborator

Resolved by #22. Any remaining features or issues should be created as new issues to track individual tasks.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants