Merge pull request #1290 from timkraut/no-useless-index-in-import-paths

[New] `no-useless-path-segments`: Add `noUselessIndex` option

README.md

@@ -256,6 +256,17 @@ A list of file extensions that will be parsed as modules and inspected for
 This defaults to `['.js']`, unless you are using the `react` shared config,
 in which case it is specified as `['.js', '.jsx']`.
 
+```js
+"settings": {
+  "import/extensions": [
+    ".js",
+    ".jsx"
+  ]
+}
+```
+
+If you require more granular extension definitions, you can use:
+
 ```js
 "settings": {
   "import/resolver": {

docs/rules/no-useless-path-segments.md

@@ -11,6 +11,9 @@ my-project
 ├── app.js
 ├── footer.js
 ├── header.js
+└── helpers.js
+└── helpers
+    └── index.js
 └── pages
     ├── about.js
     ├── contact.js
@@ -30,6 +33,8 @@ import "../pages/about.js"; // should be "./pages/about.js"
 import "../pages/about"; // should be "./pages/about"
 import "./pages//about"; // should be "./pages/about"
 import "./pages/"; // should be "./pages"
+import "./pages/index"; // should be "./pages" (except if there is a ./pages.js file)
+import "./pages/index.js"; // should be "./pages" (except if there is a ./pages.js file)
 ```
 
 The following patterns are NOT considered problems:
@@ -46,3 +51,25 @@ import ".";
 import "..";
 import fs from "fs";
 ```
+
+## Options
+
+### noUselessIndex
+
+If you want to detect unnecessary `/index` or `/index.js` (depending on the specified file extensions, see below) imports in your paths, you can enable the option `noUselessIndex`. By default it is set to `false`:
+```js
+"import/no-useless-path-segments": ["error", {
+  noUselessIndex: true,
+}]
+```
+
+Additionally to the patterns described above, the following imports are considered problems if `noUselessIndex` is enabled:
+
+```js
+// in my-project/app.js
+import "./helpers/index"; // should be "./helpers/" (not auto-fixable to `./helpers` because this would lead to an ambiguous import of `./helpers.js` and `./helpers/index.js`)
+import "./pages/index"; // should be "./pages" (auto-fixable)
+import "./pages/index.js"; // should be "./pages" (auto-fixable)
+```
+
+Note: `noUselessIndex` only avoids ambiguous imports for `.js` files if you haven't specified other resolved file extensions. See [Settings: import/extensions](https://github.com/benmosher/eslint-plugin-import#importextensions) for details.

src/rules/no-useless-path-segments.js

@@ -3,10 +3,10 @@
  * @author Thomas Grainger
  */
 
-import path from 'path'
-import sumBy from 'lodash/sumBy'
-import resolve from 'eslint-module-utils/resolve'
+import { getFileExtensions } from 'eslint-module-utils/ignore'
 import moduleVisitor from 'eslint-module-utils/moduleVisitor'
+import resolve from 'eslint-module-utils/resolve'
+import path from 'path'
 import docsUrl from '../docsUrl'
 
 /**
@@ -19,19 +19,22 @@ import docsUrl from '../docsUrl'
  * ..foo/bar -> ./..foo/bar
  * foo/bar -> ./foo/bar
  *
- * @param rel {string} relative posix path potentially missing leading './'
+ * @param relativePath {string} relative posix path potentially missing leading './'
  * @returns {string} relative posix path that always starts with a ./
  **/
-function toRel(rel) {
-  const stripped = rel.replace(/\/$/g, '')
+function toRelativePath(relativePath) {
+  const stripped = relativePath.replace(/\/$/g, '') // Remove trailing /
+
   return /^((\.\.)|(\.))($|\/)/.test(stripped) ? stripped : `./${stripped}`
 }
 
 function normalize(fn) {
-  return toRel(path.posix.normalize(fn))
+  return toRelativePath(path.posix.normalize(fn))
 }
 
-const countRelParent = x => sumBy(x, v => v === '..')
+function countRelativeParents(pathSegments) {
+  return pathSegments.reduce((sum, pathSegment) => pathSegment === '..' ? sum + 1 : sum, 0)
+}
 
 module.exports = {
   meta: {
@@ -45,6 +48,7 @@ module.exports = {
         type: 'object',
         properties: {
           commonjs: { type: 'boolean' },
+          noUselessIndex: { type: 'boolean' },
         },
         additionalProperties: false,
       },
@@ -53,57 +57,89 @@ module.exports = {
     fixable: 'code',
   },
 
-  create: function (context) {
+  create(context) {
     const currentDir = path.dirname(context.getFilename())
+    const options = context.options[0]
 
     function checkSourceValue(source) {
-      const { value } = source
+      const { value: importPath } = source
 
-      function report(proposed) {
+      function reportWithProposedPath(proposedPath) {
         context.report({
           node: source,
-          message: `Useless path segments for "${value}", should be "${proposed}"`,
-          fix: fixer => fixer.replaceText(source, JSON.stringify(proposed)),
+          // Note: Using messageIds is not possible due to the support for ESLint 2 and 3
+          message: `Useless path segments for "${importPath}", should be "${proposedPath}"`,
+          fix: fixer => proposedPath && fixer.replaceText(source, JSON.stringify(proposedPath)),
         })
       }
 
-      if (!value.startsWith('.')) {
+      // Only relative imports are relevant for this rule --> Skip checking
+      if (!importPath.startsWith('.')) {
         return
       }
 
-      const resolvedPath = resolve(value, context)
-      const normed = normalize(value)
-      if (normed !== value && resolvedPath === resolve(normed, context)) {
-        return report(normed)
+      // Report rule violation if path is not the shortest possible
+      const resolvedPath = resolve(importPath, context)
+      const normedPath = normalize(importPath)
+      const resolvedNormedPath = resolve(normedPath, context)
+      if (normedPath !== importPath && resolvedPath === resolvedNormedPath) {
+        return reportWithProposedPath(normedPath)
+      }
+
+      const fileExtensions = getFileExtensions(context.settings)
+      const regexUnnecessaryIndex = new RegExp(
+        `.*\\/index(\\${Array.from(fileExtensions).join('|\\')})?$`
+      )
+
+      // Check if path contains unnecessary index (including a configured extension)
+      if (options && options.noUselessIndex && regexUnnecessaryIndex.test(importPath)) {
+        const parentDirectory = path.dirname(importPath)
+
+        // Try to find ambiguous imports
+        if (parentDirectory !== '.' && parentDirectory !== '..') {
+          for (let fileExtension of fileExtensions) {
+            if (resolve(`${parentDirectory}${fileExtension}`, context)) {
+              return reportWithProposedPath(`${parentDirectory}/`)
+            }
+          }
+        }
+
+        return reportWithProposedPath(parentDirectory)
       }
 
-      if (value.startsWith('./')) {
+      // Path is shortest possible + starts from the current directory --> Return directly
+      if (importPath.startsWith('./')) {
         return
       }
 
+      // Path is not existing --> Return directly (following code requires path to be defined)
       if (resolvedPath === undefined) {
         return
       }
 
-      const expected = path.relative(currentDir, resolvedPath)
-      const expectedSplit = expected.split(path.sep)
-      const valueSplit = value.replace(/^\.\//, '').split('/')
-      const valueNRelParents = countRelParent(valueSplit)
-      const expectedNRelParents = countRelParent(expectedSplit)
-      const diff = valueNRelParents - expectedNRelParents
+      const expected = path.relative(currentDir, resolvedPath) // Expected import path
+      const expectedSplit = expected.split(path.sep) // Split by / or \ (depending on OS)
+      const importPathSplit = importPath.replace(/^\.\//, '').split('/')
+      const countImportPathRelativeParents = countRelativeParents(importPathSplit)
+      const countExpectedRelativeParents = countRelativeParents(expectedSplit)
+      const diff = countImportPathRelativeParents - countExpectedRelativeParents
 
+      // Same number of relative parents --> Paths are the same --> Return directly
       if (diff <= 0) {
         return
       }
 
-      return report(
-        toRel(valueSplit
-          .slice(0, expectedNRelParents)
-          .concat(valueSplit.slice(valueNRelParents + diff))
-          .join('/'))
+      // Report and propose minimal number of required relative parents
+      return reportWithProposedPath(
+        toRelativePath(
+          importPathSplit
+            .slice(0, countExpectedRelativeParents)
+            .concat(importPathSplit.slice(countImportPathRelativeParents + diff))
+            .join('/')
+        )
       )
     }
 
-    return moduleVisitor(checkSourceValue, context.options[0])
+    return moduleVisitor(checkSourceValue, options)
   },
 }

tests/src/core/ignore.js

@@ -1,6 +1,6 @@
 import { expect } from 'chai'
 
-import isIgnored, { hasValidExtension } from 'eslint-module-utils/ignore'
+import isIgnored, { getFileExtensions, hasValidExtension } from 'eslint-module-utils/ignore'
 
 import * as utils from '../utils'
 
@@ -55,4 +55,36 @@ describe('ignore', function () {
     })
   })
 
+  describe('getFileExtensions', function () {
+    it('returns a set with the file extension ".js" if "import/extensions" is not configured', function () {
+      const fileExtensions = getFileExtensions({})
+
+      expect(fileExtensions).to.include('.js')
+    })
+
+    it('returns a set with the file extensions configured in "import/extension"', function () {
+      const settings = {
+        'import/extensions': ['.js', '.jsx'],
+      }
+
+      const fileExtensions = getFileExtensions(settings)
+
+      expect(fileExtensions).to.include('.js')
+      expect(fileExtensions).to.include('.jsx')
+    })
+
+    it('returns a set with the file extensions configured in "import/extension" and "import/parsers"', function () {
+      const settings = {
+        'import/parsers': {
+          'typescript-eslint-parser': ['.ts', '.tsx'],
+        },
+      }
+
+      const fileExtensions = getFileExtensions(settings)
+
+      expect(fileExtensions).to.include('.js') // If "import/extensions" is not configured, this is the default
+      expect(fileExtensions).to.include('.ts')
+      expect(fileExtensions).to.include('.tsx')
+    })
+  })
 })