From a8bf7a0b877ad9169faa9394fae32dd6498b1dd7 Mon Sep 17 00:00:00 2001 From: Jeroen Engels Date: Mon, 18 Apr 2016 23:37:30 +0200 Subject: [PATCH] Add `import-order` rule --- CHANGELOG.md | 1 + README.md | 2 + docs/rules/import-order.md | 66 ++++++++ package.json | 1 + src/core/importType.js | 5 +- src/core/staticRequire.js | 1 + src/index.js | 1 + src/rules/import-order.js | 135 ++++++++++++++++ tests/src/core/importType.js | 22 ++- tests/src/rules/import-order.js | 266 ++++++++++++++++++++++++++++++++ 10 files changed, 484 insertions(+), 16 deletions(-) create mode 100644 docs/rules/import-order.md create mode 100644 src/rules/import-order.js create mode 100644 tests/src/rules/import-order.js diff --git a/CHANGELOG.md b/CHANGELOG.md index 81438c7689..bb937fbe92 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ This change log adheres to standards from [Keep a CHANGELOG](http://keepachangel ### Added - [`no-named-as-default-member`] to `warnings` canned config - add [`no-extraneous-dependencies`] rule ([#241], thanks [@jfmengels]) +- add [`import-order`] rule ([#247], thanks [@jfmengels]) ## [1.5.0] - 2016-04-18 ### Added diff --git a/README.md b/README.md index 6673f2282d..057f66ebc4 100644 --- a/README.md +++ b/README.md @@ -50,10 +50,12 @@ This plugin intends to support linting of ES2015+ (ES6+) import/export syntax, a * Ensure all imports appear before other statements ([`imports-first`]) * Report repeated import of the same module in multiple places ([`no-duplicates`]) * Report namespace imports ([`no-namespace`]) +* Enforce a convention in module import order ([`import-order`]) [`imports-first`]: ./docs/rules/imports-first.md [`no-duplicates`]: ./docs/rules/no-duplicates.md [`no-namespace`]: ./docs/rules/no-namespace.md +[`import-order`]: ./docs/rules/import-order.md ## Installation diff --git a/docs/rules/import-order.md b/docs/rules/import-order.md new file mode 100644 index 0000000000..01d2412e76 --- /dev/null +++ b/docs/rules/import-order.md @@ -0,0 +1,66 @@ +# 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. modules from a "parent" directory +import foo from '../foo'; +import qux from '../../foo/qux'; +// 4. "sibling" modules from the same or a sibling's directory +import bar from './bar'; +import baz from './bar/baz'; +// 5. "index" of the current directory +import main from './'; +``` + +Unassigned imports are not accounted for, as the order they are imported in may be important. + + +## 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` +``` + + +## 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'); +``` + +## Options + +This rule supports the following options: + +`order`: The order to respect. It needs to contain only and all of the following elements: `"builtin", "external", "parent", "sibling", "index"`, which is the default value. + +You can set the options like this: + +```js +"import-order/import-order": ["error", {"order": ["index", "sibling", "parent", "external", "builtin"]}] +``` diff --git a/package.json b/package.json index 33acac1f41..009dc254b5 100644 --- a/package.json +++ b/package.json @@ -72,6 +72,7 @@ "es6-symbol": "*", "eslint-import-resolver-node": "^0.2.0", "lodash.cond": "^4.3.0", + "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 81b4514449..c917c4e6f9 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' @@ -28,8 +28,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 } diff --git a/src/core/staticRequire.js b/src/core/staticRequire.js index 156dfeca45..c13c4b0664 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 7d1dd05021..d423d3df36 100644 --- a/src/index.js +++ b/src/index.js @@ -14,6 +14,7 @@ export const rules = { 'no-duplicates': require('./rules/no-duplicates'), 'imports-first': require('./rules/imports-first'), 'no-extraneous-dependencies': require('./rules/no-extraneous-dependencies'), + 'import-order': require('./rules/import-order'), // metadata-based 'no-deprecated': require('./rules/no-deprecated'), diff --git a/src/rules/import-order.js b/src/rules/import-order.js new file mode 100644 index 0000000000..43d098c5b8 --- /dev/null +++ b/src/rules/import-order.js @@ -0,0 +1,135 @@ +'use strict' + +import find from 'lodash.find' +import importType from '../core/importType' +import isStaticRequire from '../core/staticRequire' + +const defaultOrder = ['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, order, name) { + return order.indexOf(importType(name, context)) +} + +function registerNode(context, node, name, order, imported) { + const rank = computeRank(context, order, name) + if (rank !== -1) { + imported.push({name: name, rank: rank, node: node}) + } +} + +function isInVariableDeclarator(node) { + return node && + (node.type === 'VariableDeclarator' || isInVariableDeclarator(node.parent)) +} + +module.exports = function importOrderRule (context) { + const options = context.options[0] || {} + const order = options.order || defaultOrder + 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, order, imported) + } + }, + CallExpression: function handleRequires(node) { + if (level !== 0 || !isStaticRequire(node) || !isInVariableDeclarator(node.parent)) { + return + } + const name = node.arguments[0].value + registerNode(context, node, name, order, 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: { + order: { + type: 'array', + uniqueItems: true, + length: 5, + items: { + enum: defaultOrder, + }, + }, + }, + additionalProperties: false, + }, +] diff --git a/tests/src/core/importType.js b/tests/src/core/importType.js index a86ef1a440..0665b9a633 100644 --- a/tests/src/core/importType.js +++ b/tests/src/core/importType.js @@ -36,21 +36,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/import-order.js b/tests/src/rules/import-order.js new file mode 100644 index 0000000000..e194678c5c --- /dev/null +++ b/tests/src/rules/import-order.js @@ -0,0 +1,266 @@ +import { test } from '../utils' + +import { RuleTester } from 'eslint' + +const ruleTester = new RuleTester() + , rule = require('rules/import-order') + +ruleTester.run('import-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 './';`, + }), + // Default order using both require and import + test({ + code: ` + var fs = require('fs'); + import async, {foo1} from 'async'; + var relParent1 = require('../foo'); + import relParent2, {foo2} from '../foo/bar'; + var relParent3 = require('../'); + import sibling, {foo3} from './foo'; + var index = require('./');`, + }), + // 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: [{order: ['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; + `}), + ], + invalid: [ + // builtin before external module (require) + test({ + code: ` + var async = require('async'); + var fs = require('fs'); + `, + errors: [{ + ruleId: 'import-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: 'import-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: 'import-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: 'import-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: 'import-order', + message: '`../parent` import should occur before import of `./sibling`', + }], + }), + // sibling before index + test({ + code: ` + var index = require('./'); + var sibling = require('./sibling'); + `, + errors: [{ + ruleId: 'import-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: 'import-order', + message: '`async` import should occur before import of `./sibling`', + }, { + ruleId: 'import-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: 'import-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: [{order: ['index', 'sibling', 'parent', 'external', 'builtin']}], + errors: [{ + ruleId: 'import-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: 'import-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: 'import-order', + message: '`fs` import should occur before import of `./foo`', + }], + }), + ], +})