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

rule: no-unused #186

Closed
benmosher opened this issue Feb 22, 2016 · 16 comments
Closed

rule: no-unused #186

benmosher opened this issue Feb 22, 2016 · 16 comments

Comments

@benmosher
Copy link
Member

On first execution, build an index of imports from some source root(s) pattern (i.e. /src/.*\.js) and then flag all exports that are not used.

Subsequent executions using the same base would benefit from a cache.

Could try using a makefile to build the import cache in a file to maintain it across executions + build it fast? Might be a useful strategy for caching exports between executions, too.

FS cache should live in the node_modules/eslint-plugin-import folder? or just the system tmp dir?

Probably should configure base as a rule parameter? until it benefits some other rule, anyway.

Could also mark full modules unused if nothing imports a given file.

@josh
Copy link
Contributor

josh commented Apr 28, 2016

Definitely ❤️ this idea.

I've implemented something like this for @github but its pretty app specific. I was researching a way it could be extracted into a proper eslint plugin. It'd be rad if it was was part of eslint-plugin-import.

My original thought was that this plugin wouldn't have to be responsible for building the index per se. If you spec'd out the format of the index file, an application specific Makefile could be responsible for generating it. Currently we have a mix of ES6 imports and older CJS stuff. You'll also want to special case "entry points" somehow since those won't directly be imported, but maybe referenced by some html script tag in the app. Our could see our indexer looking for ES imports, CJS and these custom html templating references.

It'd still be great for a standard index builder to be part of eslint-plugin-import itself to all all the easy cases. But I'd interested in having a custom hook into that point.

@benmosher
Copy link
Member Author

My original thought was that this plugin wouldn't have to be responsible for building the index per se

I would be totally up for that if you or someone else can point me to something that would generate an index on the FS I could write the rule against. Less is more. 😎

You'll also want to special case "entry points" somehow

I'm imagining this could be a new responsibility of the resolvers, i.e. Node resolver exposes either the package.json main field(s) or root index.js, and the Webpack resolver would read the entry points out of the relevant config file(s). Could be implemented in v3 of resolvers as getEntryPoints(context|filename) => String[] or something similar.

Could also allow explicit config of entry points in a setting or rule param, but IMO the less of that, the better.

@benmosher
Copy link
Member Author

Idea: the resolvers can provide a list of entry points. Node resolver uses package.json and defaults to index.js. Webpack resolver uses "entry" field.

Using the memo parser, it shouldn't technically add much time, either.

@hjylewis
Copy link

Any news or updates on this rule? Having this rule would be super beneficial (yay, deleting code).

Let me know how I can help out!

@benmosher
Copy link
Member Author

FWIW, the no-cycle rule introduced some infrastructure that should make this straightforward.

The naïve implementation could just crawl the defined entry points' dependency graphs for all used exports, cache it using the existing cache rules, and work off of that when linting.

The real question is how time-intensive this will be:

just crawl the graphs for all used exports

Feel free to take a stab at it! (again, using no-cycle as a reference for crawling the export map graph)

@rfermann
Copy link
Contributor

Is this rule meant to report the modules themselves not being used somewhere? Or should it also/instead report individual exports (in case that there are multiple exports within a single module) without any usage? (just to make sure I don't misunderstand the aim of this rule... )

@ljharb
Copy link
Member

ljharb commented Jun 30, 2018

@rfermann every export - default or named - in every file.

The rule would also need an option so that for published packages, entry points could be exempted.

@rfermann
Copy link
Contributor

rfermann commented Jul 1, 2018

Well...too bad. I only have a prototype solution for checking the usage of the modules themselves, just as it is mentioned in #361, although created independent of this issue.

What about reopening #361 and creating 2 different rules e.g. "no-unused-modules" and "no-unused-exports"?

@ljharb
Copy link
Member

ljharb commented Jul 1, 2018

I’m not sure what you mean - you have a rule that just checks if the file paths are referenced?

@ljharb
Copy link
Member

ljharb commented Jul 1, 2018

I’m not sure i see the value of checking that a file is used or not if we already had a way to check if every export is used or not - iow, the former seems like a subset of the latter.

@rfermann
Copy link
Contributor

rfermann commented Jul 3, 2018

@ljharb: I have a first draft of a rule checking for referenced paths, yes.

In my opinion, these rules are close to each other. But I don't think, that one is a subset of the other.

Let's say, there is a module with some code in it. This module lacks any export statements (because someone deleted all export statements by accident, or for any other reason). It would pass the "no-unused-exports" rule without being reported.

@ljharb
Copy link
Member

ljharb commented Jul 4, 2018

That's a really good point.

I still think it should be the same rule tho - maybe no-unused-modules, with an "ignore" option for entry points, and separate options to control checking for specific exports, and checking files that have no exports.

@rfermann
Copy link
Contributor

rfermann commented Jul 6, 2018

That means, we will have a rule that reports:

  • unused exports (indicating, that these exports can possibly be removed from the module; option "unusedExports")
  • modules without any exports (indicating unused modules; option "missingExports")

This rule might work different than my existing solution, but I will give it a try. Although I didn't get yet, why there needs to be an "ignore" option for the entry points. Isn't that handled by the resolvers?

@ljharb
Copy link
Member

ljharb commented Jul 6, 2018

@rfermann no - published packages may have files that are never imported, but are correct and need to remain, for external consumers. Not every project being linted is an app :-)

@rfermann
Copy link
Contributor

rfermann commented Jul 7, 2018

Hm...as it turns out, I misunderstood this point. The term "entry point" led me in the wrong direction.

@ljharb
Copy link
Member

ljharb commented Apr 7, 2019

Closed with #1142.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

5 participants