From 17e29d8fe7e121dcfcbe18bd9046a003e6d97b61 Mon Sep 17 00:00:00 2001 From: Tim Kraut Date: Mon, 18 Feb 2019 11:59:08 +0100 Subject: [PATCH 1/2] Refactor no-useless-path-segments rule --- src/rules/no-useless-path-segments.js | 79 +++++++++++++++++---------- 1 file changed, 49 insertions(+), 30 deletions(-) diff --git a/src/rules/no-useless-path-segments.js b/src/rules/no-useless-path-segments.js index 2ad207fad..3fdb397c6 100644 --- a/src/rules/no-useless-path-segments.js +++ b/src/rules/no-useless-path-segments.js @@ -3,10 +3,9 @@ * @author Thomas Grainger */ -import path from 'path' -import sumBy from 'lodash/sumBy' -import resolve from 'eslint-module-utils/resolve' import moduleVisitor from 'eslint-module-utils/moduleVisitor' +import resolve from 'eslint-module-utils/resolve' +import path from 'path' import docsUrl from '../docsUrl' /** @@ -19,19 +18,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 === '..') +const countRelativeParents = (pathSegments) => pathSegments.reduce( + (sum, pathSegment) => pathSegment === '..' ? sum + 1 : sum, 0 +) module.exports = { meta: { @@ -51,59 +53,76 @@ module.exports = { ], fixable: 'code', + messages: { + uselessPath: 'Useless path segments for "{{ path }}", should be "{{ proposedPath }}"', + }, }, - create: function (context) { + create(context) { const currentDir = path.dirname(context.getFilename()) + const config = context.options[0] function checkSourceValue(source) { - const { value } = source + const { value: importPath } = source - function report(proposed) { + function report(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 => 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 report(normedPath) } - 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 } + // Report and propose minimal number of required relative parents return report( - toRel(valueSplit - .slice(0, expectedNRelParents) - .concat(valueSplit.slice(valueNRelParents + diff)) - .join('/')) + toRelativePath( + importPathSplit + .slice(0, countExpectedRelativeParents) + .concat(importPathSplit.slice(countImportPathRelativeParents + diff)) + .join('/') + ) ) } - return moduleVisitor(checkSourceValue, context.options[0]) + return moduleVisitor(checkSourceValue, config) }, } From 480894200e800d5e4f5cf90c4bf3d06d76fb2338 Mon Sep 17 00:00:00 2001 From: Tim Kraut Date: Mon, 18 Feb 2019 15:09:17 +0100 Subject: [PATCH 2/2] no-useless-path-segments: Add noUselessIndex option --- README.md | 11 ++ docs/rules/no-useless-path-segments.md | 27 +++++ src/rules/no-useless-path-segments.js | 51 ++++++---- tests/src/core/ignore.js | 34 ++++++- tests/src/rules/no-useless-path-segments.js | 107 ++++++++++++++++++-- utils/ignore.js | 1 + 6 files changed, 207 insertions(+), 24 deletions(-) diff --git a/README.md b/README.md index 41bbff0a0..7695ac368 100644 --- a/README.md +++ b/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": { diff --git a/docs/rules/no-useless-path-segments.md b/docs/rules/no-useless-path-segments.md index b2ae82a3a..6a02eab9f 100644 --- a/docs/rules/no-useless-path-segments.md +++ b/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. diff --git a/src/rules/no-useless-path-segments.js b/src/rules/no-useless-path-segments.js index 3fdb397c6..ea72e6c54 100644 --- a/src/rules/no-useless-path-segments.js +++ b/src/rules/no-useless-path-segments.js @@ -3,6 +3,7 @@ * @author Thomas Grainger */ +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' @@ -31,9 +32,9 @@ function normalize(fn) { return toRelativePath(path.posix.normalize(fn)) } -const countRelativeParents = (pathSegments) => pathSegments.reduce( - (sum, pathSegment) => pathSegment === '..' ? sum + 1 : sum, 0 -) +function countRelativeParents(pathSegments) { + return pathSegments.reduce((sum, pathSegment) => pathSegment === '..' ? sum + 1 : sum, 0) +} module.exports = { meta: { @@ -47,33 +48,28 @@ 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(context) { const currentDir = path.dirname(context.getFilename()) - const config = context.options[0] + const options = context.options[0] function checkSourceValue(source) { const { value: importPath } = source - function report(proposedPath) { + function reportWithProposedPath(proposedPath) { context.report({ node: source, - messageId: 'uselessPath', - data: { - path: importPath, - proposedPath, - }, - fix: fixer => fixer.replaceText(source, JSON.stringify(proposedPath)), + // 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)), }) } @@ -87,7 +83,28 @@ module.exports = { const normedPath = normalize(importPath) const resolvedNormedPath = resolve(normedPath, context) if (normedPath !== importPath && resolvedPath === resolvedNormedPath) { - return report(normedPath) + 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) } // Path is shortest possible + starts from the current directory --> Return directly @@ -113,7 +130,7 @@ module.exports = { } // Report and propose minimal number of required relative parents - return report( + return reportWithProposedPath( toRelativePath( importPathSplit .slice(0, countExpectedRelativeParents) @@ -123,6 +140,6 @@ module.exports = { ) } - return moduleVisitor(checkSourceValue, config) + return moduleVisitor(checkSourceValue, options) }, } diff --git a/tests/src/core/ignore.js b/tests/src/core/ignore.js index cc89f8454..8870158a5 100644 --- a/tests/src/core/ignore.js +++ b/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') + }) + }) }) diff --git a/tests/src/rules/no-useless-path-segments.js b/tests/src/rules/no-useless-path-segments.js index ed2044001..923c7efe7 100644 --- a/tests/src/rules/no-useless-path-segments.js +++ b/tests/src/rules/no-useless-path-segments.js @@ -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 }], @@ -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"'], @@ -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) diff --git a/utils/ignore.js b/utils/ignore.js index c1ddc8699..47af8122d 100644 --- a/utils/ignore.js +++ b/utils/ignore.js @@ -34,6 +34,7 @@ function makeValidExtensionSet(settings) { return exts } +exports.getFileExtensions = makeValidExtensionSet exports.default = function ignore(path, context) { // check extension whitelist first (cheap)