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

TSLint or ESLint rule #19

Open
felixfbecker opened this issue Jun 13, 2018 · 13 comments
Open

TSLint or ESLint rule #19

felixfbecker opened this issue Jun 13, 2018 · 13 comments
Labels
extension Extension or plugin for external system

Comments

@felixfbecker
Copy link

I was confused that this is not implemented as a tslint rule. It would be great to have the editor integration, line/column data for errors, shareable configuration and autofixing of tslint for this.

@vsviridov
Copy link

AFAIK, tslint operates on one file at a time.

@mrseanryan
Copy link
Collaborator

Yes tslint rules only work on 1 file at a time.

So I don't see how this could be implemented.

Closing.

@felixfbecker
Copy link
Author

Don’t tslint/typescript-eslint rules have access to the type checker which has the whole project loaded? Can that bot be used to find references?

@pzavolinsky
Copy link
Owner

We should definitely explore the linter possibility. Specially now that we have TS support in ESLint.

In raw TS, with createProgram we can access the TypeChecker. I'm not sure if that's enough to catch all usage instances but we could look into it.

@mrseanryan mrseanryan reopened this Oct 20, 2019
@mrseanryan
Copy link
Collaborator

mrseanryan commented Oct 20, 2019

(re-opened)

With tslint my understanding is, it works on 1 file at a time, and will have access to whatever types that file uses. I don't think it works by first opening the whole project. So, I don't see how a tslint rule can determine if an export is unused, without false positives ...

However - eslint (eslint-typescript) is replacing tslint.

I don't know that much about how eslint works.

There is a Visitor pattern, and a rule can implement one or more of the visiting methods.

For example, this is a simple rule, that accepts visits from Node:

https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/src/rules/camelcase.ts

This is the same general pattern used by tslint rules.


brainstorming:

Is is possible that eslint-typescript can load a whole project, before evaluating each file?

OR if it's possible to have static state across 'rule runs', then it could be possible to check exports by sharing a 'used exports' state across runs.
But this would require 2-passes: a collection pass, and then an evaluation pass.

A concern could be performance ...

@mrseanryan
Copy link
Collaborator

There is a discussion here at typescript-eslint

@monfera
Copy link

monfera commented Oct 21, 2019

Yes tslint rules only work on 1 file at a time.

File boundaries seem convenient for linter implementation but otherwise a fairly arbitrary compartmentalization of linting, because file bounds have relatively little coincidence with data flow, calls etc.

@mrseanryan mrseanryan changed the title TSLint rule TSLint or ESLint rule Nov 7, 2019
@pzavolinsky
Copy link
Owner

pzavolinsky commented Nov 7, 2019

I was reading a little bit about ESLint rules/plugins yesterday and I don't think we can access multiple files inside a rule. More specifically I found this:
image

That being said, if we completely ignore that ☝️ then we might get away with something. If we (pre-)generate the analysis for the whole project and we could incrementally update/replace everything related to the current file, we might get away with a workable prototype.

Given that we start the POC by blatantly ignoring everything that is considered a good practice from the ESLint POV this might not be an ideal solution but it just might work.

A brutal first approach would be to run a a full ts-unused-exports analysis for each file. Performance will be laughable (specially in big projects) but we could quickly check if this as a viable solution. If it is, we could start thinking on incremental analysis (something that will be required to get this done).

@felixfbecker
Copy link
Author

I'd recommend looking to integrate with typescript-eslint. typescript-eslint has many rules that use type information, which means it does have access to the whole project, because otherwise it wouldn't be able to make use of full type information (which needs resolving types from other files). This also means there is a way for rules to get a reference to the type checker/project, and hopefully that would be enough to implement this rule.

@mrseanryan mrseanryan added the extension Extension or plugin for external system label Jan 4, 2020
@maneetgoyal
Copy link

maneetgoyal commented Mar 8, 2020

Hi all, will the new rule offer something better than the approach described here (typescript-eslint/typescript-eslint#371 (comment)). I understand that approach requires some configuration but apart from that, does this new rule seem to offer any other benefits?


Edit: Upon testing, that approach was catching unused-exports of functions and variables but not the interfaces.

@mrseanryan
Copy link
Collaborator

@maneetgoyal the approach in that comment seems more suited to eslint architecture. (Parser and Resolver).

But either way sounds like a rewrite

@cscleison
Copy link

You might try this https://github.com/benmosher/eslint-plugin-import/blob/master/docs/rules/no-unused-modules.md

It worked like a charm for me. The extends config with "plugin:import/typescript" was essential for me, otherwise the lint would take forever and return nothing. If you do any absolute webpack config, check their docs for the resolvers.

@Zamiell
Copy link

Zamiell commented Jun 14, 2021

Thanks @cscleison, that rule is working perfectly for me!

RE: "plugin:import/typescript"
For people who are using the Airbnb TypeScript Config, then you don't have to do anything extra, it just works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extension Extension or plugin for external system
Projects
None yet
Development

No branches or pull requests

8 participants