[Fix] order: allow secondary alphabetical sorting

randallreedjr · ljharb · commit 6a4156d9862c · 2017-02-13T17:46:58.000-05:00
Fixes import-js#389.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -9,6 +9,7 @@ This change log adheres to standards from [Keep a CHANGELOG](http://keepachangel
 ### Added
 - Add ESLint 5 support ([#1122], thanks [@ai] and [@ljharb])
 - Add [`no-relative-parent-imports`] rule: disallow relative imports from parent directories ([#1093], thanks [@chrislloyd])
+- [`order`] allows secondary alphabetical sort ([#389])
 
 ### Fixed
 - `namespace` rule: ensure it works in eslint 5/ecmaVersion 2018 (thanks [@ljharb])
diff --git a/docs/rules/order.md b/docs/rules/order.md
@@ -164,6 +164,41 @@ import index from './';
 import sibling from './foo';
 ```
 
+### `sort: [ignore|alphabetical]`:
+
+Enforces alphabetical sorting within import groups:
+
+- If set to `ignore`, no errors related to order within import groups will be reported (default).
+- If set to `alphabetical`, imports within a group must be alphabetized. Imports across groups will not be compared. Alphabetical sort ignores capitalization.
+
+With the default group setting, the following will be invalid:
+
+```js
+/* eslint import/order: ["error", {"sort": "alphabetical"}] */
+import path from 'path';
+import fs from 'fs';
+import index from './';
+import sibling from './foo';
+```
+
+while this will be valid:
+
+```js
+/* eslint import/order: ["error", {"sort": "alphabetical"}] */
+import fs from 'fs';
+import path from 'path';
+import index from './';
+import sibling from './foo';
+```
+
+```js
+/* eslint import/order: ["error", {"sort": "ignore"}] */
+import path from 'path';
+import fs from 'fs';
+import index from './';
+import sibling from './foo';
+```
+
 ## Related
 
 - [`import/external-module-folders`] setting
diff --git a/src/rules/order.js b/src/rules/order.js
@@ -8,6 +8,7 @@ const defaultGroups = ['builtin', 'external', 'parent', 'sibling', 'index']
 
 // REPORTING AND FIXING
 
+
 function reverse(array) {
   return array.map(function (v) {
     return {
@@ -72,14 +73,14 @@ function takeTokensBeforeWhile(sourceCode, node, condition) {
   return result.reverse()
 }
 
-function findOutOfOrder(imported) {
+function findOutOfOrder(imported, comparator) {
   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) {
+    const res = comparator(importedModule, maxSeenRankNode)
+    if (comparator(maxSeenRankNode, importedModule)) {
       maxSeenRankNode = importedModule
     }
     return res
@@ -215,32 +216,58 @@ function fixOutOfOrder(context, firstNode, secondNode, order) {
   }
 }
 
-function reportOutOfOrder(context, imported, outOfOrder, order) {
-  outOfOrder.forEach(function (imp) {
-    const found = imported.find(function hasHigherRank(importedItem) {
-      return importedItem.rank > imp.rank
-    })
-    fixOutOfOrder(context, found, imp, order)
-  })
+function reportOutOfOrder(context, sortedImports, outOfOrder, order, comparator) {
+  // Pass in imports pre-sorted to ensure `found` is correct position
+  for (let imp of outOfOrder) {
+    const found = sortedImports.find(importedItem => comparator(importedItem, imp))
+
+    context.report(imp.node, '`' + imp.name + '` import should occur ' + order +
+      ' import of `' + found.name + '`')
+  }
 }
 
-function makeOutOfOrderReport(context, imported) {
-  const outOfOrder = findOutOfOrder(imported)
+function makeOutOfOrderReport(context, imported, forwardSortComparator, reverseSortComparator) {
+  const outOfOrder = findOutOfOrder(imported, reverseSortComparator)
   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)
+  const reversedImported = [...imported].reverse()
+  const reversedOrder = findOutOfOrder(reversedImported, forwardSortComparator)
+  const sortedImports = [...imported].sort(forwardSortComparator)
   if (reversedOrder.length < outOfOrder.length) {
-    reportOutOfOrder(context, reversedImported, reversedOrder, 'after')
-    return
+    reportOutOfOrder(context,
+      sortedImports.reverse(),
+      reversedOrder,
+      'after',
+      reverseSortComparator
+    )
+  } else {
+    reportOutOfOrder(context,
+      sortedImports,
+      outOfOrder,
+      'before',
+      forwardSortComparator
+    )
   }
-  reportOutOfOrder(context, imported, outOfOrder, 'before')
 }
 
 // DETECTING
 
+function determineComparators(alphabetize) {
+  let forwardSortComparator, reverseSortComparator
+  if (alphabetize) {
+    forwardSortComparator = (a, b) => a.rank > b.rank ||
+      (a.rank === b.rank && (a.name.toLowerCase() > b.name.toLowerCase()))
+    reverseSortComparator = (a, b) => a.rank < b.rank ||
+      (a.rank === b.rank && (a.name.toLowerCase() < b.name.toLowerCase()))
+  } else {
+    forwardSortComparator = (a, b) => a.rank > b.rank
+    reverseSortComparator = (a, b) => a.rank < b.rank
+  }
+  return [forwardSortComparator, reverseSortComparator]
+}
+
 function computeRank(context, ranks, name, type) {
   return ranks[importType(name, context)] +
     (type === 'import' ? 0 : 100)
@@ -382,13 +409,16 @@ module.exports = {
               'never',
             ],
           },
+          'sort': {
+            enum: [ 'ignore', 'alphabetical' ],
+          },
         },
         additionalProperties: false,
       },
     ],
   },
 
-  create: function importOrderRule (context) {
+  create: function importOrderRule(context) {
     const options = context.options[0] || {}
     const newlinesBetweenImports = options['newlines-between'] || 'ignore'
     let ranks
@@ -428,7 +458,9 @@ module.exports = {
         registerNode(context, node, name, 'require', ranks, imported)
       },
       'Program:exit': function reportAndReset() {
-        makeOutOfOrderReport(context, imported)
+      	const alphabetize = (options['sort'] === 'alphabetical')
+        const [forwardSortComparator, reverseSortComparator] = determineComparators(alphabetize)
+        makeOutOfOrderReport(context, imported, forwardSortComparator, reverseSortComparator)
 
         if (newlinesBetweenImports !== 'ignore') {
           makeNewlinesBetweenReport(context, imported, newlinesBetweenImports)
diff --git a/tests/src/rules/order.js b/tests/src/rules/order.js
@@ -3,7 +3,7 @@ import { test } from '../utils'
 import { RuleTester } from 'eslint'
 
 const ruleTester = new RuleTester()
-    , rule = require('rules/order')
+  , rule = require('rules/order')
 
 function withoutAutofixOutput(test) {
   return Object.assign({}, test, { output: test.code })
@@ -21,7 +21,7 @@ ruleTester.run('order', rule, {
         var relParent3 = require('../');
         var sibling = require('./foo');
         var index = require('./');`,
-      }),
+    }),
     // Default order using import
     test({
       code: `
@@ -32,7 +32,7 @@ ruleTester.run('order', rule, {
         import relParent3 from '../';
         import sibling, {foo3} from './foo';
         import index from './';`,
-      }),
+    }),
     // Multiple module of the same rank next to each other
     test({
       code: `
@@ -41,7 +41,7 @@ ruleTester.run('order', rule, {
         var path = require('path');
         var _ = require('lodash');
         var async = require('async');`,
-      }),
+    }),
     // Overriding order to be the reverse of the default order
     test({
       code: `
@@ -136,9 +136,9 @@ ruleTester.run('order', rule, {
         var relParent1 = require('../foo');
       `,
       options: [{groups: [
-        ['builtin', 'index'],
-        ['sibling', 'parent', 'external'],
-      ]}],
+          ['builtin', 'index'],
+          ['sibling', 'parent', 'external'],
+        ]}],
     }),
     // Omitted types should implicitly be considered as the last type
     test({
@@ -147,10 +147,10 @@ ruleTester.run('order', rule, {
         var path = require('path');
       `,
       options: [{groups: [
-        'index',
-        ['sibling', 'parent', 'external'],
-        // missing 'builtin'
-      ]}],
+          'index',
+          ['sibling', 'parent', 'external'],
+          // missing 'builtin'
+        ]}],
     }),
     // Mixing require and import should have import up top
     test({
@@ -486,12 +486,12 @@ ruleTester.run('order', rule, {
     // fix order with windows end of lines
     test({
       code:
-        `/* comment0 */  /* comment1 */  var async = require('async'); /* comment2 */` + `\r\n` +
-        `/* comment3 */  var fs = require('fs'); /* comment4 */` + `\r\n`
+      `/* comment0 */  /* comment1 */  var async = require('async'); /* comment2 */` + `\r\n` +
+      `/* comment3 */  var fs = require('fs'); /* comment4 */` + `\r\n`
       ,
       output:
-        `/* comment3 */  var fs = require('fs'); /* comment4 */` + `\r\n` +
-        `/* comment0 */  /* comment1 */  var async = require('async'); /* comment2 */` + `\r\n`
+      `/* comment3 */  var fs = require('fs'); /* comment4 */` + `\r\n` +
+      `/* comment0 */  /* comment1 */  var async = require('async'); /* comment2 */` + `\r\n`
       ,
       errors: [{
         ruleId: 'order',
@@ -740,9 +740,9 @@ ruleTester.run('order', rule, {
         var sibling = require('./foo');
       `,
       options: [{groups: [
-        ['builtin', 'index'],
-        ['sibling', 'parent', 'external'],
-      ]}],
+          ['builtin', 'index'],
+          ['sibling', 'parent', 'external'],
+        ]}],
       errors: [{
         ruleId: 'order',
         message: '`path` import should occur before import of `./foo`',
@@ -759,10 +759,10 @@ ruleTester.run('order', rule, {
         var path = require('path');
       `,
       options: [{groups: [
-        'index',
-        ['sibling', 'parent', 'external', 'internal'],
-        // missing 'builtin'
-      ]}],
+          'index',
+          ['sibling', 'parent', 'external', 'internal'],
+          // missing 'builtin'
+        ]}],
       errors: [{
         ruleId: 'order',
         message: '`async` import should occur before import of `path`',
@@ -776,9 +776,9 @@ ruleTester.run('order', rule, {
         var index = require('./');
       `,
       options: [{groups: [
-        'index',
-        ['sibling', 'parent', 'UNKNOWN', 'internal'],
-      ]}],
+          'index',
+          ['sibling', 'parent', 'UNKNOWN', 'internal'],
+        ]}],
       errors: [{
         ruleId: 'order',
         message: 'Incorrect configuration of the rule: Unknown type `"UNKNOWN"`',
@@ -791,9 +791,9 @@ ruleTester.run('order', rule, {
         var index = require('./');
       `,
       options: [{groups: [
-        'index',
-        ['sibling', 'parent', ['builtin'], 'internal'],
-      ]}],
+          'index',
+          ['sibling', 'parent', ['builtin'], 'internal'],
+        ]}],
       errors: [{
         ruleId: 'order',
         message: 'Incorrect configuration of the rule: Unknown type `["builtin"]`',
@@ -806,9 +806,9 @@ ruleTester.run('order', rule, {
         var index = require('./');
       `,
       options: [{groups: [
-        'index',
-        ['sibling', 'parent', 2, 'internal'],
-      ]}],
+          'index',
+          ['sibling', 'parent', 2, 'internal'],
+        ]}],
       errors: [{
         ruleId: 'order',
         message: 'Incorrect configuration of the rule: Unknown type `2`',
@@ -821,9 +821,9 @@ ruleTester.run('order', rule, {
         var index = require('./');
       `,
       options: [{groups: [
-        'index',
-        ['sibling', 'parent', 'parent', 'internal'],
-      ]}],
+          'index',
+          ['sibling', 'parent', 'parent', 'internal'],
+        ]}],
       errors: [{
         ruleId: 'order',
         message: 'Incorrect configuration of the rule: `parent` is duplicated',
