diff --git a/CHANGELOG.md b/CHANGELOG.md index 27dfc8775..6959a8469 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ This change log adheres to standards from [Keep a CHANGELOG](http://keepachangel - add [`no-extraneous-dependencies`] rule ([#241], thanks [@jfmengels]) - add [`extensions`] rule ([#250], thanks [@lo1tuma]) - add [`no-nodejs-modules`] rule ([#261], thanks [@jfmengels]) +- add [`order`] rule ([#247], thanks [@jfmengels]) - consider `resolve.fallback` config option in the webpack resolver ([#254]) ### Changed @@ -155,9 +156,6 @@ for info on changes for earlier releases. [`imports-first`]: ./docs/rules/imports-first.md [`no-nodejs-modules`]: ./docs/rules/no-nodejs-modules.md -[#256]: https://github.com/benmosher/eslint-plugin-import/pull/256 -[#254]: https://github.com/benmosher/eslint-plugin-import/pull/254 -[#250]: https://github.com/benmosher/eslint-plugin-import/pull/250 [#243]: https://github.com/benmosher/eslint-plugin-import/pull/243 [#241]: https://github.com/benmosher/eslint-plugin-import/pull/241 [#239]: https://github.com/benmosher/eslint-plugin-import/pull/239 @@ -198,5 +196,3 @@ for info on changes for earlier releases. [@singles]: https://github.com/singles [@jfmengels]: https://github.com/jfmengels [@dmnd]: https://github.com/dmnd -[@lo1tuma]: https://github.com/lo1tuma -[@lemonmade]: https://github.com/lemonmade diff --git a/README.md b/README.md index d7deb38fe..67d63d779 100644 --- a/README.md +++ b/README.md @@ -53,11 +53,13 @@ This plugin intends to support linting of ES2015+ (ES6+) import/export syntax, a * Report repeated import of the same module in multiple places ([`no-duplicates`]) * Report namespace imports ([`no-namespace`]) * Ensure consistent use of file extension within the import path ([`extensions`]) +* Enforce a convention in module import order ([`order`]) [`imports-first`]: ./docs/rules/imports-first.md [`no-duplicates`]: ./docs/rules/no-duplicates.md [`no-namespace`]: ./docs/rules/no-namespace.md [`extensions`]: ./docs/rules/extensions.md +[`order`]: ./docs/rules/order.md ## Installation diff --git a/docs/rules/order.md b/docs/rules/order.md new file mode 100644 index 000000000..ef5a95393 --- /dev/null +++ b/docs/rules/order.md @@ -0,0 +1,91 @@ +# Enforce a convention in module import order + +Enforce a convention in the order of `require()` / `import` statements. The order is as shown in the following example: + +```js +// 1. node "builtins" +import fs from 'fs'; +import path from 'path'; +// 2. "external" modules +import _ from 'lodash'; +import chalk from 'chalk'; +// 3. "internal" modules +// (if you have configured your path or webpack to handle your internal paths differently) +import foo from 'src/foo'; +// 4. modules from a "parent" directory +import foo from '../foo'; +import qux from '../../foo/qux'; +// 5. "sibling" modules from the same or a sibling's directory +import bar from './bar'; +import baz from './bar/baz'; +// 6. "index" of the current directory +import main from './'; +``` + +Unassigned imports are ignored, as the order they are imported in may be important. + +Statements using the ES6 `import` syntax must appear before any `require()` statements. + + +## Fail + +```js +import _ from 'lodash'; +import path from 'path'; // `path` import should occur before import of `lodash` + +// ----- + +var _ = require('lodash'); +var path = require('path'); // `path` import should occur before import of `lodash` + +// ----- + +var path = require('path'); +import foo from './foo'; // `import` statements must be before `require` statement +``` + + +## Pass + +```js +import path from 'path'; +import _ from 'lodash'; + +// ----- + +var path = require('path'); +var _ = require('lodash'); + +// ----- + +// Allowed as ̀`babel-register` is not assigned. +require('babel-register'); +var path = require('path'); + +// ----- + +// Allowed as `import` must be before `require` +import foo from './foo'; +var path = require('path'); +``` + +## Options + +This rule supports the following options: + +`groups`: How groups are defined, and the order to respect. `groups` must be an array of `string` or [`string`]. The only allowed `string`s are: `"builtin"`, `"external"`, `"internal"`, `"parent"`, `"sibling"`, `"index"`. The enforced order is the same as the order of each element in a group. Omitted types are implicitly grouped together as the last element. Example: +```js +[ + 'builtin', // Built-in types are first + ['sibling', 'parent'], // Then sibling and parent types. They can be mingled together + 'index', // Then the index file + // Then the rest: internal and external type +] +``` +The default value is `["builtin", "external", "internal", "parent", "sibling", "index"]`. + +You can set the options like this: + +```js +"import/order": ["error", {"groups": ["index", "sibling", "parent", "internal", "external", "builtin"]}] +``` diff --git a/package.json b/package.json index 70e6599d2..7962abc56 100644 --- a/package.json +++ b/package.json @@ -73,6 +73,7 @@ "eslint-import-resolver-node": "^0.2.0", "lodash.cond": "^4.3.0", "lodash.endswith": "^4.0.1", + "lodash.find": "^4.3.0", "object-assign": "^4.0.1", "pkg-up": "^1.0.0" } diff --git a/src/core/importType.js b/src/core/importType.js index 81b451444..133acec60 100644 --- a/src/core/importType.js +++ b/src/core/importType.js @@ -1,6 +1,6 @@ import cond from 'lodash.cond' import builtinModules from 'builtin-modules' -import { basename, join } from 'path' +import { join } from 'path' import resolve from './resolve' @@ -18,7 +18,12 @@ function isExternalModule(name, path) { return (!path || -1 < path.indexOf(join('node_modules', name))) } -function isProjectModule(name, path) { +const scopedRegExp = /^@\w+\/\w+/ +function isScoped(name) { + return scopedRegExp.test(name) +} + +function isInternalModule(name, path) { if (!externalModuleRegExp.test(name)) return false return (path && -1 === path.indexOf(join('node_modules', name))) } @@ -28,8 +33,7 @@ function isRelativeToParent(name) { } const indexFiles = ['.', './', './index', './index.js'] -function isIndex(name, path) { - if (path) return basename(path).split('.')[0] === 'index' +function isIndex(name) { return indexFiles.indexOf(name) !== -1 } @@ -40,7 +44,8 @@ function isRelativeToSibling(name) { const typeTest = cond([ [isBuiltIn, constant('builtin')], [isExternalModule, constant('external')], - [isProjectModule, constant('project')], + [isScoped, constant('external')], + [isInternalModule, constant('internal')], [isRelativeToParent, constant('parent')], [isIndex, constant('index')], [isRelativeToSibling, constant('sibling')], diff --git a/src/core/staticRequire.js b/src/core/staticRequire.js index 156dfeca4..c13c4b066 100644 --- a/src/core/staticRequire.js +++ b/src/core/staticRequire.js @@ -1,6 +1,7 @@ // todo: merge with module visitor export default function isStaticRequire(node) { return node && + node.callee && node.callee.type === 'Identifier' && node.callee.name === 'require' && node.arguments.length === 1 && diff --git a/src/index.js b/src/index.js index 733363811..50f6ebfbf 100644 --- a/src/index.js +++ b/src/index.js @@ -16,6 +16,7 @@ export const rules = { 'imports-first': require('./rules/imports-first'), 'no-extraneous-dependencies': require('./rules/no-extraneous-dependencies'), 'no-nodejs-modules': require('./rules/no-nodejs-modules'), + 'order': require('./rules/order'), // metadata-based 'no-deprecated': require('./rules/no-deprecated'), diff --git a/src/rules/order.js b/src/rules/order.js new file mode 100644 index 000000000..980c6534e --- /dev/null +++ b/src/rules/order.js @@ -0,0 +1,175 @@ +'use strict' + +import find from 'lodash.find' +import importType from '../core/importType' +import isStaticRequire from '../core/staticRequire' + +const defaultGroups = ['builtin', 'external', 'parent', 'sibling', 'index'] + +// REPORTING + +function reverse(array) { + return array.map(function (v) { + return { + name: v.name, + rank: -v.rank, + node: v.node, + } + }).reverse() +} + +function findOutOfOrder(imported) { + if (imported.length === 0) { + return [] + } + let maxSeenRankNode = imported[0] + return imported.filter(function (importedModule) { + const res = importedModule.rank < maxSeenRankNode.rank + if (maxSeenRankNode.rank < importedModule.rank) { + maxSeenRankNode = importedModule + } + return res + }) +} + +function report(context, imported, outOfOrder, order) { + outOfOrder.forEach(function (imp) { + const found = find(imported, function hasHigherRank(importedItem) { + return importedItem.rank > imp.rank + }) + context.report(imp.node, '`' + imp.name + '` import should occur ' + order + + ' import of `' + found.name + '`') + }) +} + +function makeReport(context, imported) { + const outOfOrder = findOutOfOrder(imported) + if (!outOfOrder.length) { + return + } + // There are things to report. Try to minimize the number of reported errors. + const reversedImported = reverse(imported) + const reversedOrder = findOutOfOrder(reversedImported) + if (reversedOrder.length < outOfOrder.length) { + report(context, reversedImported, reversedOrder, 'after') + return + } + report(context, imported, outOfOrder, 'before') +} + +// DETECTING + +function computeRank(context, ranks, name, type) { + return ranks[importType(name, context)] + + (type === 'import' ? 0 : 100) +} + +function registerNode(context, node, name, type, ranks, imported) { + const rank = computeRank(context, ranks, name, type) + if (rank !== -1) { + imported.push({name, rank, node}) + } +} + +function isInVariableDeclarator(node) { + return node && + (node.type === 'VariableDeclarator' || isInVariableDeclarator(node.parent)) +} + +const types = ['builtin', 'external', 'internal', 'parent', 'sibling', 'index'] + +// Creates an object with type-rank pairs. +// Example: { index: 0, sibling: 1, parent: 1, external: 1, builtin: 2, internal: 2 } +// Will throw an error if it contains a type that does not exist, or has a duplicate +function convertGroupsToRanks(groups) { + const rankObject = groups.reduce(function(res, group, index) { + if (typeof group === 'string') { + group = [group] + } + group.forEach(function(groupItem) { + if (types.indexOf(groupItem) === -1) { + throw new Error('Incorrect configuration of the rule: Unknown type `' + + JSON.stringify(groupItem) + '`') + } + if (res[groupItem] !== undefined) { + throw new Error('Incorrect configuration of the rule: `' + groupItem + '` is duplicated') + } + res[groupItem] = index + }) + return res + }, {}) + + const omittedTypes = types.filter(function(type) { + return rankObject[type] === undefined + }) + + return omittedTypes.reduce(function(res, type) { + res[type] = groups.length + return res + }, rankObject) +} + +module.exports = function importOrderRule (context) { + const options = context.options[0] || {} + let ranks + + try { + ranks = convertGroupsToRanks(options.groups || defaultGroups) + } catch (error) { + // Malformed configuration + return { + Program: function(node) { + context.report(node, error.message) + }, + } + } + let imported = [] + let level = 0 + + function incrementLevel() { + level++ + } + function decrementLevel() { + level-- + } + + return { + ImportDeclaration: function handleImports(node) { + if (node.specifiers.length) { // Ignoring unassigned imports + const name = node.source.value + registerNode(context, node, name, 'import', ranks, imported) + } + }, + CallExpression: function handleRequires(node) { + if (level !== 0 || !isStaticRequire(node) || !isInVariableDeclarator(node.parent)) { + return + } + const name = node.arguments[0].value + registerNode(context, node, name, 'require', ranks, imported) + }, + 'Program:exit': function reportAndReset() { + makeReport(context, imported) + imported = [] + }, + FunctionDeclaration: incrementLevel, + FunctionExpression: incrementLevel, + ArrowFunctionExpression: incrementLevel, + BlockStatement: incrementLevel, + 'FunctionDeclaration:exit': decrementLevel, + 'FunctionExpression:exit': decrementLevel, + 'ArrowFunctionExpression:exit': decrementLevel, + 'BlockStatement:exit': decrementLevel, + } +} + +module.exports.schema = [ + { + type: 'object', + properties: { + groups: { + type: 'array', + }, + }, + additionalProperties: false, + }, +] diff --git a/tests/src/core/importType.js b/tests/src/core/importType.js index a86ef1a44..88de5eacb 100644 --- a/tests/src/core/importType.js +++ b/tests/src/core/importType.js @@ -22,9 +22,14 @@ describe('importType(name)', function () { expect(importType('lodash/fp', context)).to.equal('external') }) - it("should return 'project' for non-builtins resolved outside of node_modules", function () { + it("should return 'external' for scopes packages", function() { + expect(importType('@cycle/core', context)).to.equal('external') + expect(importType('@cycle/dom', context)).to.equal('external') + }) + + it("should return 'internal' for non-builtins resolved outside of node_modules", function () { const pathContext = testContext({ "import/resolver": { node: { paths: [ path.join(__dirname, '..', '..', 'files') ] } } }) - expect(importType('importType', pathContext)).to.equal('project') + expect(importType('importType', pathContext)).to.equal('internal') }) it("should return 'parent' for internal modules that go through the parent", function() { @@ -36,21 +41,17 @@ describe('importType(name)', function () { it("should return 'sibling' for internal modules that are connected to one of the siblings", function() { expect(importType('./foo', context)).to.equal('sibling') expect(importType('./foo/bar', context)).to.equal('sibling') + expect(importType('./importType', context)).to.equal('sibling') + expect(importType('./importType/', context)).to.equal('sibling') + expect(importType('./importType/index', context)).to.equal('sibling') + expect(importType('./importType/index.js', context)).to.equal('sibling') }) - describe("should return 'index' for sibling index file when", function() { - it("resolves", function() { - expect(importType('./importType', context)).to.equal('index') - expect(importType('./importType/', context)).to.equal('index') - expect(importType('./importType/index', context)).to.equal('index') - expect(importType('./importType/index.js', context)).to.equal('index') - }) - it("doesn't resolve", function() { - expect(importType('.', context)).to.equal('index') - expect(importType('./', context)).to.equal('index') - expect(importType('./index', context)).to.equal('index') - expect(importType('./index.js', context)).to.equal('index') - }) + describe("should return 'index' for sibling index file", function() { + expect(importType('.', context)).to.equal('index') + expect(importType('./', context)).to.equal('index') + expect(importType('./index', context)).to.equal('index') + expect(importType('./index.js', context)).to.equal('index') }) it("should return 'unknown' for any unhandled cases", function() { diff --git a/tests/src/rules/order.js b/tests/src/rules/order.js new file mode 100644 index 000000000..645cddf0c --- /dev/null +++ b/tests/src/rules/order.js @@ -0,0 +1,417 @@ +import { test } from '../utils' + +import { RuleTester } from 'eslint' + +const ruleTester = new RuleTester() + , rule = require('rules/order') + +ruleTester.run('order', rule, { + valid: [ + // Default order using require + test({ + code: ` + var fs = require('fs'); + var async = require('async'); + var relParent1 = require('../foo'); + var relParent2 = require('../foo/bar'); + var relParent3 = require('../'); + var sibling = require('./foo'); + var index = require('./');`, + }), + // Default order using import + test({ + code: ` + import fs from 'fs'; + import async, {foo1} from 'async'; + import relParent1 from '../foo'; + import relParent2, {foo2} from '../foo/bar'; + import relParent3 from '../'; + import sibling, {foo3} from './foo'; + import index from './';`, + }), + // Multiple module of the same rank next to each other + test({ + code: ` + var fs = require('fs'); + var fs = require('fs'); + var path = require('path'); + var _ = require('lodash'); + var async = require('async');`, + }), + // Overriding order to be the reverse of the default order + test({ + code: ` + var index = require('./'); + var sibling = require('./foo'); + var relParent3 = require('../'); + var relParent2 = require('../foo/bar'); + var relParent1 = require('../foo'); + var async = require('async'); + var fs = require('fs'); + `, + options: [{groups: ['index', 'sibling', 'parent', 'external', 'builtin']}], + }), + // Ignore dynamic requires + test({ + code: ` + var path = require('path'); + var _ = require('lodash'); + var async = require('async'); + var fs = require('f' + 's');`, + }), + // Ignore non-require call expressions + test({ + code: ` + var path = require('path'); + var result = add(1, 2); + var _ = require('lodash');`, + }), + // Ignore requires that are not at the top-level + test({ + code: ` + var index = require('./'); + function foo() { + var fs = require('fs'); + } + () => require('fs'); + if (a) { + require('fs'); + }`, + }), + // Ignore unknown/invalid cases + test({ + code: ` + var unknown1 = require('/unknown1'); + var fs = require('fs'); + var unknown2 = require('/unknown2'); + var async = require('async'); + var unknown3 = require('/unknown3'); + var foo = require('../foo'); + var unknown4 = require('/unknown4'); + var bar = require('../foo/bar'); + var unknown5 = require('/unknown5'); + var parent = require('../'); + var unknown6 = require('/unknown6'); + var foo = require('./foo'); + var unknown7 = require('/unknown7'); + var index = require('./'); + var unknown8 = require('/unknown8'); + `}), + // Ignoring unassigned values by default (require) + test({ + code: ` + require('./foo'); + require('fs'); + var path = require('path'); + `}), + // Ignoring unassigned values by default (import) + test({ + code: ` + import './foo'; + import 'fs'; + import path from 'path'; + `}), + // No imports + test({ + code: ` + function add(a, b) { + return a + b; + } + var foo; + `}), + // Grouping import types + test({ + code: ` + var fs = require('fs'); + var index = require('./'); + var path = require('path'); + + var sibling = require('./foo'); + var relParent3 = require('../'); + var async = require('async'); + var relParent1 = require('../foo'); + `, + options: [{groups: [ + ['builtin', 'index'], + ['sibling', 'parent', 'external'], + ]}], + }), + // Omitted types should implicitly be considered as the last type + test({ + code: ` + var index = require('./'); + var path = require('path'); + `, + options: [{groups: [ + 'index', + ['sibling', 'parent', 'external'], + // missing 'builtin' + ]}], + }), + // Mixing require and import should have import up top + test({ + code: ` + import async, {foo1} from 'async'; + import relParent2, {foo2} from '../foo/bar'; + import sibling, {foo3} from './foo'; + var fs = require('fs'); + var relParent1 = require('../foo'); + var relParent3 = require('../'); + var index = require('./'); + `, + }), + ], + invalid: [ + // builtin before external module (require) + test({ + code: ` + var async = require('async'); + var fs = require('fs'); + `, + errors: [{ + ruleId: 'order', + message: '`fs` import should occur before import of `async`', + }], + }), + // builtin before external module (import) + test({ + code: ` + import async from 'async'; + import fs from 'fs'; + `, + errors: [{ + ruleId: 'order', + message: '`fs` import should occur before import of `async`', + }], + }), + // builtin before external module (mixed import and require) + test({ + code: ` + var async = require('async'); + import fs from 'fs'; + `, + errors: [{ + ruleId: 'order', + message: '`fs` import should occur before import of `async`', + }], + }), + // external before parent + test({ + code: ` + var parent = require('../parent'); + var async = require('async'); + `, + errors: [{ + ruleId: 'order', + message: '`async` import should occur before import of `../parent`', + }], + }), + // parent before sibling + test({ + code: ` + var sibling = require('./sibling'); + var parent = require('../parent'); + `, + errors: [{ + ruleId: 'order', + message: '`../parent` import should occur before import of `./sibling`', + }], + }), + // sibling before index + test({ + code: ` + var index = require('./'); + var sibling = require('./sibling'); + `, + errors: [{ + ruleId: 'order', + message: '`./sibling` import should occur before import of `./`', + }], + }), + // Multiple errors + test({ + code: ` + var sibling = require('./sibling'); + var async = require('async'); + var fs = require('fs'); + `, + errors: [{ + ruleId: 'order', + message: '`async` import should occur before import of `./sibling`', + }, { + ruleId: 'order', + message: '`fs` import should occur before import of `./sibling`', + }], + }), + // Uses 'after' wording if it creates less errors + test({ + code: ` + var index = require('./'); + var fs = require('fs'); + var path = require('path'); + var _ = require('lodash'); + var foo = require('foo'); + var bar = require('bar'); + `, + errors: [{ + ruleId: 'order', + message: '`./` import should occur after import of `bar`', + }], + }), + // Overriding order to be the reverse of the default order + test({ + code: ` + var fs = require('fs'); + var index = require('./'); + `, + options: [{groups: ['index', 'sibling', 'parent', 'external', 'builtin']}], + errors: [{ + ruleId: 'order', + message: '`./` import should occur before import of `fs`', + }], + }), + // member expression of require + test({ + code: ` + var foo = require('./foo').bar; + var fs = require('fs'); + `, + errors: [{ + ruleId: 'order', + message: '`fs` import should occur before import of `./foo`', + }], + }), + // nested member expression of require + test({ + code: ` + var foo = require('./foo').bar.bar.bar; + var fs = require('fs'); + `, + errors: [{ + ruleId: 'order', + message: '`fs` import should occur before import of `./foo`', + }], + }), + // Grouping import types + test({ + code: ` + var fs = require('fs'); + var index = require('./'); + var sibling = require('./foo'); + var path = require('path'); + `, + options: [{groups: [ + ['builtin', 'index'], + ['sibling', 'parent', 'external'], + ]}], + errors: [{ + ruleId: 'order', + message: '`path` import should occur before import of `./foo`', + }], + }), + // Omitted types should implicitly be considered as the last type + test({ + code: ` + var path = require('path'); + var async = require('async'); + `, + options: [{groups: [ + 'index', + ['sibling', 'parent', 'external', 'internal'], + // missing 'builtin' + ]}], + errors: [{ + ruleId: 'order', + message: '`async` import should occur before import of `path`', + }], + }), + // Setting the order for an unknown type + // should make the rule trigger an error and do nothing else + test({ + code: ` + var async = require('async'); + var index = require('./'); + `, + options: [{groups: [ + 'index', + ['sibling', 'parent', 'UNKNOWN', 'internal'], + ]}], + errors: [{ + ruleId: 'order', + message: 'Incorrect configuration of the rule: Unknown type `"UNKNOWN"`', + }], + }), + // Type in an array can't be another array, too much nesting + test({ + code: ` + var async = require('async'); + var index = require('./'); + `, + options: [{groups: [ + 'index', + ['sibling', 'parent', ['builtin'], 'internal'], + ]}], + errors: [{ + ruleId: 'order', + message: 'Incorrect configuration of the rule: Unknown type `["builtin"]`', + }], + }), + // No numbers + test({ + code: ` + var async = require('async'); + var index = require('./'); + `, + options: [{groups: [ + 'index', + ['sibling', 'parent', 2, 'internal'], + ]}], + errors: [{ + ruleId: 'order', + message: 'Incorrect configuration of the rule: Unknown type `2`', + }], + }), + // Duplicate + test({ + code: ` + var async = require('async'); + var index = require('./'); + `, + options: [{groups: [ + 'index', + ['sibling', 'parent', 'parent', 'internal'], + ]}], + errors: [{ + ruleId: 'order', + message: 'Incorrect configuration of the rule: `parent` is duplicated', + }], + }), + // Mixing require and import should have import up top + test({ + code: ` + import async, {foo1} from 'async'; + import relParent2, {foo2} from '../foo/bar'; + var fs = require('fs'); + var relParent1 = require('../foo'); + var relParent3 = require('../'); + import sibling, {foo3} from './foo'; + var index = require('./'); + `, + errors: [{ + ruleId: 'order', + message: '`./foo` import should occur before import of `fs`', + }], + }), + test({ + code: ` + var fs = require('fs'); + import async, {foo1} from 'async'; + import relParent2, {foo2} from '../foo/bar'; + `, + errors: [{ + ruleId: 'order', + message: '`fs` import should occur after import of `../foo/bar`', + }], + }), + ], +})