Skip to content

Commit

Permalink
Merge a8242dd into 651829d
Browse files Browse the repository at this point in the history
  • Loading branch information
@timkraut
timkraut committed Mar 4, 2019
2 parents 651829d + a8242dd commit 3886465
Show file tree
Hide file tree
Showing 5 changed files with 212 additions and 37 deletions.
11 changes: 11 additions & 0 deletions README.md
View file
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
26 changes: 26 additions & 0 deletions docs/rules/no-useless-path-segments.md
View file
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,24 @@ 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)
```

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.
104 changes: 73 additions & 31 deletions src/rules/no-useless-path-segments.js
View file
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,65 +48,104 @@ module.exports = {
type: 'object',
properties: {
commonjs: { type: 'boolean' },
noUselessIndex: { type: 'boolean' },
},
additionalProperties: false,
},
],

fixable: 'code',
messages: {
uselessPath: 'Useless path segments for "{{ path }}", should be "{{ proposedPath }}"',
},
},

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)),
messageId: 'uselessPath',
data: {
path: importPath,
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)
},
}
107 changes: 101 additions & 6 deletions tests/src/rules/no-useless-path-segments.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,20 +7,31 @@ const rule = require('rules/no-useless-path-segments')
function runResolverTests(resolver) {
ruleTester.run(`no-useless-path-segments (${resolver})`, rule, {
valid: [
// commonjs with default options
// CommonJS modules with default options
test({ code: 'require("./../files/malformed.js")' }),

// esmodule
// ES modules with default options
test({ code: 'import "./malformed.js"' }),
test({ code: 'import "./test-module"' }),
test({ code: 'import "./bar/"' }),
test({ code: 'import "."' }),
test({ code: 'import ".."' }),
test({ code: 'import fs from "fs"' }),
test({ code: 'import fs from "fs"' }),

// ES modules + noUselessIndex
test({ code: 'import "../index"' }), // noUselessIndex is false by default
test({ code: 'import "../my-custom-index"', options: [{ noUselessIndex: true }] }),
test({ code: 'import "./bar.js"', options: [{ noUselessIndex: true }] }), // ./bar/index.js exists
test({ code: 'import "./bar"', options: [{ noUselessIndex: true }] }),
test({ code: 'import "./bar/"', options: [{ noUselessIndex: true }] }), // ./bar.js exists
test({ code: 'import "./malformed.js"', options: [{ noUselessIndex: true }] }), // ./malformed directory does not exist
test({ code: 'import "./malformed"', options: [{ noUselessIndex: true }] }), // ./malformed directory does not exist
test({ code: 'import "./importType"', options: [{ noUselessIndex: true }] }), // ./importType.js does not exist
],

invalid: [
// commonjs
// CommonJS modules
test({
code: 'require("./../files/malformed.js")',
options: [{ commonjs: true }],
Expand Down Expand Up @@ -62,7 +73,49 @@ function runResolverTests(resolver) {
errors: [ 'Useless path segments for "./deep//a", should be "./deep/a"'],
}),

// esmodule
// CommonJS modules + noUselessIndex
test({
code: 'require("./bar/index.js")',
options: [{ commonjs: true, noUselessIndex: true }],
errors: ['Useless path segments for "./bar/index.js", should be "./bar/"'], // ./bar.js exists
}),
test({
code: 'require("./bar/index")',
options: [{ commonjs: true, noUselessIndex: true }],
errors: ['Useless path segments for "./bar/index", should be "./bar/"'], // ./bar.js exists
}),
test({
code: 'require("./importPath/")',
options: [{ commonjs: true, noUselessIndex: true }],
errors: ['Useless path segments for "./importPath/", should be "./importPath"'], // ./importPath.js does not exist
}),
test({
code: 'require("./importPath/index.js")',
options: [{ commonjs: true, noUselessIndex: true }],
errors: ['Useless path segments for "./importPath/index.js", should be "./importPath"'], // ./importPath.js does not exist
}),
test({
code: 'require("./importType/index")',
options: [{ commonjs: true, noUselessIndex: true }],
errors: ['Useless path segments for "./importType/index", should be "./importType"'], // ./importPath.js does not exist
}),
test({
code: 'require("./index")',
options: [{ commonjs: true, noUselessIndex: true }],
errors: ['Useless path segments for "./index", should be "."'],
}),
test({
code: 'require("../index")',
options: [{ commonjs: true, noUselessIndex: true }],
errors: ['Useless path segments for "../index", should be ".."'],
}),
test({
code: 'require("../index.js")',
options: [{ commonjs: true, noUselessIndex: true }],
errors: ['Useless path segments for "../index.js", should be ".."'],
}),

// ES modules
test({
code: 'import "./../files/malformed.js"',
errors: [ 'Useless path segments for "./../files/malformed.js", should be "../files/malformed.js"'],
Expand Down Expand Up @@ -95,8 +148,50 @@ function runResolverTests(resolver) {
code: 'import "./deep//a"',
errors: [ 'Useless path segments for "./deep//a", should be "./deep/a"'],
}),
],
})

// ES modules + noUselessIndex
test({
code: 'import "./bar/index.js"',
options: [{ noUselessIndex: true }],
errors: ['Useless path segments for "./bar/index.js", should be "./bar/"'], // ./bar.js exists
}),
test({
code: 'import "./bar/index"',
options: [{ noUselessIndex: true }],
errors: ['Useless path segments for "./bar/index", should be "./bar/"'], // ./bar.js exists
}),
test({
code: 'import "./importPath/"',
options: [{ noUselessIndex: true }],
errors: ['Useless path segments for "./importPath/", should be "./importPath"'], // ./importPath.js does not exist
}),
test({
code: 'import "./importPath/index.js"',
options: [{ noUselessIndex: true }],
errors: ['Useless path segments for "./importPath/index.js", should be "./importPath"'], // ./importPath.js does not exist
}),
test({
code: 'import "./importPath/index"',
options: [{ noUselessIndex: true }],
errors: ['Useless path segments for "./importPath/index", should be "./importPath"'], // ./importPath.js does not exist
}),
test({
code: 'import "./index"',
options: [{ noUselessIndex: true }],
errors: ['Useless path segments for "./index", should be "."'],
}),
test({
code: 'import "../index"',
options: [{ noUselessIndex: true }],
errors: ['Useless path segments for "../index", should be ".."'],
}),
test({
code: 'import "../index.js"',
options: [{ noUselessIndex: true }],
errors: ['Useless path segments for "../index.js", should be ".."'],
}),
],
})
}

['node', 'webpack'].forEach(runResolverTests)
1 change: 1 addition & 0 deletions utils/ignore.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ function makeValidExtensionSet(settings) {

return exts
}
exports.getFileExtensions = makeValidExtensionSet

exports.default = function ignore(path, context) {
// check extension whitelist first (cheap)
Expand Down

0 comments on commit 3886465

Please sign in to comment.