Completely rework group-exports implementation

import-js · May 2, 2017 · 22e103c · 22e103c
1 parent 05aad15
commit 22e103c
Show file tree

Hide file tree

Showing 2 changed files with 106 additions and 54 deletions.
diff --git a/src/rules/group-exports.js b/src/rules/group-exports.js
@@ -2,96 +2,107 @@ const meta = {}
 /* eslint-disable max-len */
 const errors = {
   ExportNamedDeclaration: 'Multiple named export declarations; consolidate all named exports into a single export declaration',
-  MemberExpression: 'Multiple CommonJS exports; consolidate all exports into a single assignment to `module.exports`',
+  AssignmentExpression: 'Multiple CommonJS exports; consolidate all exports into a single assignment to `module.exports`',
 }
 /* eslint-enable max-len */
-const parents = [
-  'AssignmentExpression',
-  'MemberExpression',
-]
 
 /**
- * Determine how many property accesses precede this node
+ * Returns an array with names of the properties in the accessor chain for MemberExpression nodes
  *
- * For example, `module.exports` = 1, `module.exports.something` = 2 and so on.
+ * Example:
  *
- * @param     {Object}    node    The node being visited
- * @return    {Number}
+ * `module.exports = {}` => ['module', 'exports']
+ * `module.exports.property = true` => ['module', 'exports', 'property']
+ *
+ * @param     {Node}    node    AST Node (MemberExpression)
+ * @return    {Array}           Array with the property names in the chain
  * @private
  */
-function accessorDepth(node) {
-  let depth = 0
+function accessorChain(node) {
+  const chain = []
 
-  while (node.type === 'MemberExpression') {
-    depth++
-    node = node.parent
-  }
+  do {
+    chain.unshift(node.property.name)
+
+    if (node.object.type === 'Identifier') {
+      chain.unshift(node.object.name)
+      break
+    }
 
-  return depth
+    node = node.object
+  } while (node.type === 'MemberExpression')
+
+  return chain
 }
 
 function create(context) {
-  const named = new Set()
-  const commonjs = {
-    defaultExportIsObject: false,
+  const nodes = {
+    modules: new Set(),
+    commonjs: {
+      named: new Set(),
+      default: null,
+    },
   }
 
   return {
     ExportNamedDeclaration(node) {
-      named.add(node)
+      nodes.modules.add(node)
     },
 
-    MemberExpression(node) {
-      const parent = node.parent
-
-      // These are not the parents you are looking for
-      if (parents.indexOf(parent.type) === -1) {
+    AssignmentExpression(node) {
+      if (node.left.type !== 'MemberExpression') {
         return
       }
 
-      if (parent.type === 'AssignmentExpression') {
-        // Member expressions on the right side of the assignment do not interest us
-        if (parent.left !== node) {
+      const chain = accessorChain(node.left)
+
+      // Assignments to module.exports
+      if (chain[0] === 'module' && chain[1] === 'exports') {
+        // Adding properties to module.exports (module.exports.* = *)
+        if (chain.length === 3) {
+          nodes.commonjs.named.add(node)
           return
         }
 
-        // Special case: when assigning an object literal to `module.exports`, treat it as a named
-        // export declaration
-        if (parent.right.type === 'ObjectExpression') {
-          commonjs.defaultExportIsObject = true
-          named.add(node)
+        // Direct assignments to module.exports (module.exports = *)
+        if (chain.length === 2) {
+          // Assigning an object literal is treated as a named export declaration
+          if (node.right.type === 'ObjectExpression') {
+            nodes.commonjs.named.add(node)
+            return
+          }
+
+          // Assigning anything else (string, function, bool) is treated as a default export
+          // declaration
+          nodes.commonjs.default = node
           return
         }
-      }
 
-      const depth = accessorDepth(node)
-
-      // Assignments to module.exports
-      // Only treat assignments to `module.exports` object as named exports, ie.
-      // module.exports.named = true
-      // And ignore direct assignments and assignments more deep, ie.
-      // module.exports = {}
-      // module.exports.named.property = true
-      if (node.object.name === 'module' && node.property.name === 'exports' && depth === 2) {
-        named.add(node)
-        return
+        // Deeper assignments are ignored since they just modify what's already being exported
       }
 
-      // Assignments to exports
-      if (node.object.name === 'exports' && depth === 1) {
-        named.add(node)
+      // Assignments to exports (exports.* = *)
+      if (chain[0] === 'exports' && chain.length === 2) {
+        nodes.commonjs.named.add(node)
         return
       }
     },
 
     'Program:exit': function onExit() {
-      if (named.size > 1) {
-        for (const node of named) {
-          // If the "default" CommonJS export was not an object literal, do not report anything
-          if (node.type !== 'ExportNamedDeclaration' && !commonjs.defaultExportIsObject) {
-            continue
-          }
+      // Report multiple `export` declarations (ES2015 modules)
+      if (nodes.modules.size > 1) {
+        for (const node of nodes.modules) {
+          context.report({
+            node,
+            message: errors[node.type],
+          })
+        }
+      }
 
+      // Report multiple `module.exports` assignments (CommonJS) unless the module contains a
+      // "default" export (ie. it exports something else than object literal - a string, fn, etc.)
+      if (!nodes.commonjs.default && nodes.commonjs.named.size > 1) {
+        for (const node of nodes.commonjs.named) {
           context.report({
             node,
             message: errors[node.type],

diff --git a/tests/src/rules/group-exports.js b/tests/src/rules/group-exports.js
@@ -76,6 +76,11 @@ ruleTester.run('group-exports', rule, {
       module.exports = () => {}
       module.exports.attached = true
     ` }),
+    test({ code: `
+      module.exports = () => {}
+      exports.test = true
+      exports.another = true
+    ` }),
     test({ code: `
       module.exports = "non-object"
       module.exports.attached = true
@@ -96,6 +101,22 @@ ruleTester.run('group-exports', rule, {
       const another = true
       export default {}
     ` }),
+    test({ code: `
+      module.something.else = true
+      module.something.different = true
+    ` }),
+    test({ code: `
+      module.exports.test = true
+      module.something.different = true
+    ` }),
+    test({ code: `
+      exports.test = true
+      module.something.different = true
+    ` }),
+    test({ code: `
+      unrelated = 'assignment'
+      module.exports.test = true
+    ` }),
   ],
   invalid: [
     test({
@@ -140,5 +161,25 @@ ruleTester.run('group-exports', rule, {
         errors.commonjs,
       ],
     }),
+    test({
+      code: `
+        module.exports.test = true
+        module.exports.another = true
+      `,
+      errors: [
+        errors.commonjs,
+        errors.commonjs,
+      ],
+    }),
+    test({
+      code: `
+        exports.test = true
+        module.exports.another = true
+      `,
+      errors: [
+        errors.commonjs,
+        errors.commonjs,
+      ],
+    }),
   ],
 })