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

No useless index in import paths #1290

Merged
merged 2 commits into from
Mar 16, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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": {
Expand Down
27 changes: 27 additions & 0 deletions docs/rules/no-useless-path-segments.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@ my-project
├── app.js
├── footer.js
├── header.js
└── helpers.js
└── helpers
└── index.js
└── pages
├── about.js
├── contact.js
Expand All @@ -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:
Expand All @@ -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)
```
timkraut marked this conversation as resolved.
Show resolved Hide resolved

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.
98 changes: 67 additions & 31 deletions src/rules/no-useless-path-segments.js
Original file line number Diff line number Diff line change
Expand Up @@ -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'

/**
Expand All @@ -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: {
Expand All @@ -45,6 +48,7 @@ module.exports = {
type: 'object',
properties: {
commonjs: { type: 'boolean' },
noUselessIndex: { type: 'boolean' },
},
additionalProperties: false,
},
Expand All @@ -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)),
ljharb marked this conversation as resolved.
Show resolved Hide resolved
})
}

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)
},
}
34 changes: 33 additions & 1 deletion tests/src/core/ignore.js
Original file line number Diff line number Diff line change
@@ -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'

Expand Down Expand Up @@ -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')
})
})
})
Loading