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

Remove ESLint support #601

Closed
piotr-oles opened this issue Apr 24, 2021 · 10 comments
Closed

Remove ESLint support #601

piotr-oles opened this issue Apr 24, 2021 · 10 comments
Labels
eslint Related to ESLint

Comments

@piotr-oles
Copy link
Collaborator

I'm thinking about removing ESLint support from this plugin in the next major release.

Context

When I created the first version of this plugin, I added TSLint support. I did it because TSLint was able to consume TypeScript's AST. This meant that we were parsing TypeScript files only once and then used AST's in both TypeScript and TSLint. This was a clear performance benefit.
Later, TSLint was deprecated, and we added ESLint as a substitute for existing features of the plugin. We can't re-use TypeScript's AST in the ESLint so there is no performance benefit.

Issues

Having ESLint support in the plugin:

  • increases maintenance costs - I have limited time to work on this project and I would prefer to focus on the type-checking feature
  • complicates our code-base - we have to abstract TypeScript and ESLint to handle them regardless of API differences
  • breaks the single-responsibility principle - the name of the plugin is fork-ts-checker-webpack-plugin, not fork-es-linter-webpack-plugin

There is also https://github.com/webpack-contrib/eslint-webpack-plugin which looks like a nice replacement for this feature.
What do you think @johnnyreilly?

@piotr-oles piotr-oles added the eslint Related to ESLint label Apr 24, 2021
@johnnyreilly
Copy link
Member

johnnyreilly commented Apr 24, 2021

Hey @piotr-oles

Here's my thoughts:

complicates our code-base - we have to abstract TypeScript and ESLint to handle them regardless of API differences

It does, but this is already completed work and the abstraction isn't giant.

breaks the single-responsibility principle - the name of the plugin is fork-ts-checker-webpack-plugin, not fork-es-linter-webpack-plugin

For my money these linting and type checking are similar enough to be considered as not really violating this principle. Just my opinion.

increases maintenance costs - I have limited time to work on this project and I would prefer to focus on the type-checking feature

For me this is the clincher. If ESLint support is costing you time, and given you're the main person supporting the project at the moment, I think you should feel free to drop ESLint support.

You're right, there's other tools out there that can do this. And in fact tools like create-react-app already use those in favour of the inbuilt fork-ts-checker-webpack-plugin functionality.

So go for it!

drop it like it's hot

@piotr-oles piotr-oles pinned this issue Apr 24, 2021
@grumd
Copy link

grumd commented Apr 27, 2021

Can't you use Typescript's AST to get ESLint errors with something like this? https://gist.github.com/Quramy/d1473603c1e00132d71de573eb5d2cae

@grumd
Copy link

grumd commented Apr 29, 2021

I also want to mention that only two alternatives exist, eslint-loader and eslint-webpack-plugin. Both of which aren't running in a separate process and significantly slow down the compilation. I'll probably just be using older versions of fork-ts-checker if eslint support is dropped. Actually in the light of this post I've tried using eslint-webpack-plugin in our codebase, and it ate 21Gb of my RAM for no reason while also making the compilation 2-3x slower.

@piotr-oles
Copy link
Collaborator Author

piotr-oles commented Apr 30, 2021

That's not true - eslint-webpack-plugin uses worker-thread :) It may not be perfect as all software - did you report an issue about memory consumption?

@Jaraujo6
Copy link

Jaraujo6 commented May 5, 2021

I just ran a series of builds with eslint-webpack-plugin with thread: true and it doubled my build time from about 2.67min to 5.5min. And this is with a large, deployed, project. I sympathize if you feel you can't maintain ESLint support, but please know that this plugin is most definitely not a good substitute

@piotr-oles
Copy link
Collaborator Author

Thanks for the feedback :) I will not remove older version of the plugin, so you will still be able to use it ;)

@Jaraujo6
Copy link

Jaraujo6 commented May 5, 2021

Thank you for all your work :)

@kim3er
Copy link

kim3er commented Dec 21, 2021

I'll miss ESLint support, it's a key feature of the plugin. I have yet to get eslint-webpack-plugin working in my setup, yet your plugin worked straight out of the box. I suspect the decision is too far down the line, but I hope you reconsider.

@piotr-oles piotr-oles unpinned this issue Jan 29, 2022
@eamodio
Copy link
Contributor

eamodio commented Jan 29, 2022

While I can certainly understand your reasoning (and really appreciate your work), I have to admit I'm saddened by this removal. With v6 and eslint, my build used to take ~24s, now with v7 and eslint-webpack-plugin it takes ~40s 😢 I've tried using the threading option as well, but then it never succeeds. I don't know what you were doing before vs what eslint-webpack-plugin is doing (maybe you can suggest to them to pick up your previous code?)

Also while small, if I remove eslint from both v6 and v7 build, v7 is consistently taking 1s longer.

@piotr-oles
Copy link
Collaborator Author

I think the main difference is that they start to lint files after all modules are processed (to get a list of changed/deleted files).
In this plugin, we start linting right after file changes.
I will propose this solution for eslint-webpack-plugin

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
eslint Related to ESLint
Projects
None yet
Development

No branches or pull requests

6 participants