No useless index in import paths

[timkraut]

As discussed in #394, this PR adds an option noUselessIndex to detect (and fix) paths containing unnecessary index parts (with or without file extension):

Given the following file structure:

my-project
├── foo.js
└── foo
    └── index.js
└── bar
    └── index.js

Before:

import 'foo/index' // No warning
import 'bar/index' // No warning

After:

import './foo/index.js' // Warning (fixable to `./foo/`)
import './foo/index' // Warning (fixable to `./foo/`)
import './bar/index' // Warning (fixable to `./bar`)

Implementation detail:

I've replaced the sumBy function with reduce to get rid of Lodash (not necessary in this case IMO). A benchmark which compares these 2 alternatives shows no difference between them or a slight advantage for reduce: https://www.measurethat.net/Benchmarks/ShowResult/46787

Fixes #394.

[ljharb]

Let's put this behind an option, to avoid it being a breaking change.

[timkraut]

@ljharb I've updated the PR to use an option detectUselessIndex to detect these issue. The rule tries to find ambiguous imports for all extensions provided in:

    'import/resolver': {
      'node': {
        'extensions': [
          // some extensions
        ],
      },
    },

If this setting is not set, ['.js'] will be assumed.

Let me know if that looks like a reasonable approach to you. I'm totally open for different ideas.

The tests are still failing. I'm not sure what to do about them. To make sure my changes pass the tests, I've only used the test suite for no-useless-path-segments locally. So this should be fine once the other failing tests have been fixed.

[coveralls]

Coverage increased (+0.02%) to 97.336% when pulling 4808942 on timkraut:no-useless-index-in-import-paths into 246be82 on benmosher:master.

[coveralls]

Coverage decreased (-0.5%) to 93.651% when pulling 29ad42c on timkraut:no-useless-index-in-import-paths into bdc05aa on benmosher:master.

[coveralls]

Coverage decreased (-0.5%) to 93.651% when pulling 29ad42c on timkraut:no-useless-index-in-import-paths into bdc05aa on benmosher:master.

[ljharb]

this should be autofixable to ./helpers/

[timkraut]

Ah. I didn't even know that there is a difference between ./helpers/ --> ./helpers/index.js and ./helpers --> ./helpers.js.

Just to make sure we don't miss an option, these are all possible import paths:

./helpers.js --> ./helpers.js
./helpers --> ./helpers.js
./helpers/ --> ./helpers/index.js
./helpers/index.js --> ./helpers/index.js
./helpers/index --> ./helpers/index.js
./helpers/index/ --> ./helpers/index/index.js

If I'm not mistaken, this means that there is no possibility for ambiguous imports. Either it's an index.js file in an directory (in this case, we have a trailing /) or it's a file (in this case, we have no trailing /). Am I missing something?

[ljharb]

Yes.

In the presence of both helpers.js and helpers/index.js:

./helpers.js → ./helpers.js
./helpers → ./helpers.js (ambiguous)
./helpers/ → ./helpers/index.js
./helpers/index.js → ./helpers/index.js
./helpers/index → ./helpers/index.js
/.helpers/index/ → ./helpers/index/index.js (error, nonexistent here)

When there is only helpers.js:

./helpers.js → ./helpers.js
./helpers → ./helpers.js
./helpers/ → ./helpers/index.js (error, nonexistent here)
./helpers/index.js → ./helpers/index.js (error, nonexistent here)
./helpers/index → ./helpers/index.js (error, nonexistent here)
/.helpers/index/ → ./helpers/index/index.js (error, nonexistent here)

When there is only helpers/index.js:

./helpers.js → ./helpers.js  (error, nonexistent here)
./helpers → ./helpers/index.js
./helpers/ → ./helpers/index.js
./helpers/index.js → ./helpers/index.js
./helpers/index → ./helpers/index.js
/.helpers/index/ → ./helpers/index/index.js (error, nonexistent here)

Everything that exists is autofixable except the one ambiguous case.

[timkraut]

Thank you very much! That helped a lot!

I've extended the list by the expected result:

helpers.js and helpers/index.js exist

Import path (import '...')	Resolves to	Shortest possible path? (Explanation)
./helpers.js	./helpers.js	✅ (changing the path to ./helpers would make it ambiguous)
./helpers	./helpers.js (ambiguous)	✅ (path is ambiguous, but still decidedly resolves to the file, not the directory)
./helpers/	./helpers/index.js	✅ (changing the path to ./helpers would make it ambiguous)
./helpers/index.js	./helpers/index.js	❌ (change to ./helpers/ possible)
./helpers/index	./helpers/index.js	❌ (change to ./helpers/ possible)
./helpers/index/	./helpers/index/index.js	(Not existent)

Only helpers.js exists:

Import path (import '...')	Resolves to	Shortest possible path? (Explanation)
./helpers.js	./helpers.js	✅ (no useless index part in this path - .js is not needed but could be detected by extensions rule)
./helpers	./helpers.js	✅
./helpers/	./helpers.js	(Not existent)
./helpers/index.js	./helpers/index.js	(Not existent)
./helpers/index	./helpers/index.js	(Not existent)
./helpers/index/	./helpers/index/index.js	(Not existent)

Only helpers/index.js exists:

Import path (import '...')	Resolves to	Shortest possible path? (Explanation)
./helpers.js	./helpers.js	(Not existent)
./helpers	./helpers/index.js	✅
./helpers/	./helpers/index.js	❌ (change to ./helpers possible)
./helpers/index.js	./helpers/index.js	❌ (change to ./helpers possible)
./helpers/index	./helpers/index.js	❌ (change to ./helpers possible)
./helpers/index/	./helpers/index/index.js	(Not existent)

I've added tests for all these cases and adapted the implementation to that. Would you prefer to see a dedicated warning for the ambiguous case or should this just be accepted as a valid import?

[ljharb]

I've updated your table for two cases, otherwise looks good.

[timkraut]

I've changed the name of the new option from detectUselessIndex to noUselessIndex and rebased the branch to the latest master commit.

[timkraut]

I've updated the PR to include tests for getFileExtensions. As this function returns a Set, I've updated chai.js to the latest version to test for that. This resulted in failing tests which I've fixed as well. See chaijs/chai#781 for the breaking changes (in the case of this plugin, I had to rename .deep to .nested)

[timkraut]

I'm struggling a little bit with the support for ESLint 2 and 3. I've fixed the issue that message is missing which resulted in failing tests. ESLint 4 und 5 seem to understand the messageId property. But now newer versions of ESLint fail because I've provided both properties... Is there a way to detect the ESLint version so that I can use the new property for message (messageId) while still keep the support for old ESLint versions (message)?

It looks like the other failing tests are mostly due to the TypeScript/alternate parsers support which isn't working in ESLint 2 and 3 apparently.

I get messages like the following:

errors:
   [ TypeError: traverser.traverse is not a function
         at Object.parseForESLint (/Users/timkraut/Development/eslint-plugin-import/node_modules/typescript-eslint-parser/parser.js:41:15)
         at Object.exports.parse (/Users/timkraut/Development/eslint-plugin-import/node_modules/typescript-eslint-parser/parser.js:79:17)
         at parse (/Users/timkraut/Development/eslint-plugin-import/utils/parse.js:38:17)
         at Function.ExportMap.parse (/Users/timkraut/Development/eslint-plugin-import/src/ExportMap.js:339:15)
         at Function.ExportMap.for (/Users/timkraut/Development/eslint-plugin-import/src/ExportMap.js:323:25)
         at Function.ExportMap.get (/Users/timkraut/Development/eslint-plugin-import/src/ExportMap.js:286:10)
         at Context.<anonymous> (/Users/timkraut/Development/eslint-plugin-import/tests/src/core/getExports.js:332:31)
         at callFn (/Users/timkraut/Development/eslint-plugin-import/node_modules/mocha/lib/runnable.js:348:21)
         at Hook.Runnable.run (/Users/timkraut/Development/eslint-plugin-import/node_modules/mocha/lib/runnable.js:340:7)
         at next (/Users/timkraut/Development/eslint-plugin-import/node_modules/mocha/lib/runner.js:309:10)
         at Immediate.<anonymous> (/Users/timkraut/Development/eslint-plugin-import/node_modules/mocha/lib/runner.js:339:5)
         at processImmediate (timers.js:632:19) ],

Do you have an idea how I might fix those?

coveralls says that the test coverage decreased with this commit in the file no-cycle.js. I don't understand why this is the case as I haven't touched this file. If you have any idea, let me know.

[ljharb]

@timkraut i've pulled out your chai change into a separate commit on master (it'll be pushed shortly)

If older eslints don't support messageId, let's not use it yet.

[timkraut]

@ljharb I've removed the messageId property. Apparently, this was causing all the other failures. I guess from my side it's ok to be merged.

[timkraut]

Is there anything else I can do to get this merged/released?

[timkraut]

@ljharb Could you release this, too? Currently the documentation states something which isn't possible with the release from january.

[ljharb]

Nope, only one person can release this package.