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

Proposal: disallow "index" in import paths #394

Closed
lencioni opened this issue Jun 22, 2016 · 16 comments · Fixed by #1290
Closed

Proposal: disallow "index" in import paths #394

lencioni opened this issue Jun 22, 2016 · 16 comments · Fixed by #1290

Comments

@lencioni
Copy link
Contributor

Importing the directory will resolve to index, so including index in your path is unnecessary. I think this would make a good rule for this project, what do you think?

Bad:

import foo from 'foo/index';

Good:

import foo from 'foo';
@ljharb
Copy link
Member

ljharb commented Jun 22, 2016

This should work in concert with resolution, so that when foo.js exists, foo/index is ok.

@lencioni
Copy link
Contributor Author

I think that should be an option. Some folks might want to prevent foo.js and foo/index.js from coexisting.

@benmosher
Copy link
Member

I think it makes sense. (no-explicit-index?)

There is a similar co-existence issue with package.json defining main as something other than index.js.

I think it would make sense to ignore the edge cases for the first implementation.

There is always // eslint-disable-line, which has the nice property of implying that the line author really meant to do something that the rule would otherwise report.

@ljharb
Copy link
Member

ljharb commented Jun 23, 2016

I think it would be very dangerous to ignore the case of foo.js and foo/index.js both existing, which would mean that complying with this lint rule would silently cause different code to be imported.

@benmosher
Copy link
Member

@ljharb I feel like that case would be better solved by a "no-ambiguous-imports" rule. Such a rule would report an import with >1 possible file given all of the extensions, path folders, magic indexes, package mains, etc.

This no-index rule, as stated, is basically stylistic. And I agree, it would work best in tandem with another rule that protects against possible ambiguous resolved module paths.

@ljharb
Copy link
Member

ljharb commented Jun 26, 2016

It's not just stylistic - in tandem with a separate rule, it'd be fine - but as I implied in #394 (comment), "stylistic" goes out the window when complying with the style suggestion would silently change the way your program runs.

Even if my project allows ambiguous imports, it might still prefer to omit "index" when possible, thus I'd have that other rule disabled - if this rule warned on "foo/index" when "foo" exists, then nothing but a ♯ eye would be able to prevent me accidentally importing the wrong thing, as even the resolver rule would happily accept it.

Put another way, the airbnb config couldn't possibly ever enable this rule as requested, unless it never, under any circumstances recommended a change that would result in a different file being imported.

@benmosher
Copy link
Member

Fair enough. Would be easy enough to resolve both of "whatever/index" and "whatever" and do something different (nothing? different message?) if the two paths don't match.

This rule would be easily auto-fix-able too, I suspect. But yeah, you certainly wouldn't want to auto fix to a different module.

@ljharb
Copy link
Member

ljharb commented Jun 26, 2016

I think that it absolutely should be easily autofixable, and you wouldn't want to autofix to a different module, which is my exact objection :-)

If that's handled, then I think this rule would be an amazing addition, along with a separate no-ambiguous-imports rule like you mentioned :-D

@ljharb
Copy link
Member

ljharb commented May 21, 2018

This is largely addressed by no-useless-path-segments; I'm not sure we need a separate rule solely for "index" imports?

@robwierzbowski
Copy link

No useless path segments works for my use case. Disallowing import file names other than index feels like a very specific use case -- is it widespread enough to warrant inclusion in this plugin?

No offense meant, I just haven't worked on any projects that followed that rule without exception.

@ljharb
Copy link
Member

ljharb commented Dec 19, 2018

To clarify; Airbnb follows this rule without exception, but no-useless-path-segments is how we enforce it.

@timkraut
Copy link
Contributor

timkraut commented Feb 15, 2019

We are currently looking for a way to find imports with an useless /index. I'd happily invest some time to create a rule for that or extend an existing one. Which solution would you prefer?

New rule no-explicit-index (maybe alongside an additional rule no-ambiguous-imports?) or adding it as an option to no-useless-path-segments?

@ljharb
Copy link
Member

ljharb commented Feb 15, 2019

I thought no-useless-path-segments already covers this - is there a use case for forbidding index without also forbidding useless path segments? If it doesn’t cover it, it should; if there’s no use case, this can then be closed.

@timkraut
Copy link
Contributor

timkraut commented Feb 18, 2019

It looks like it doesn't do this yet. I currently don't see a reasonable use case where one but not the other one is required. The only thing we should pay attention to is not to rewrite ambiguous imports. I'll work on a PR to add this to no-useless-path-segments.

@timkraut
Copy link
Contributor

timkraut commented Feb 18, 2019

I have some issues with the setup on macOS. If I clone the repo, run npm install or yarn and npm run test or yarn test afterwards, I get 42 failing tests. I've got a message that I'm using an unsupported TypeScript version so I tried to install the version which is reported to be working but this didn't change anything. Any idea what I might be missing?

Edit: Looks like the same tests are also failing in the pipeline of the PR.

@ljharb
Copy link
Member

ljharb commented Feb 18, 2019

cc @benmosher for failing tests on master

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

Successfully merging a pull request may close this issue.

5 participants