Update: Refactor no-js-extension rule

* Add meta docs * Improve test coverage * Replace `hasJsExtension` method with native `endsWith`
cvisco · Aug 18, 2016 · 9c401ba · 9c401ba
1 parent ca2bb1e
commit 9c401ba
Show file tree

Hide file tree

Showing 3 changed files with 49 additions and 37 deletions.
diff --git a/lib/rules/no-js-extension.js b/lib/rules/no-js-extension.js
@@ -1,15 +1,29 @@
 /**
- * @fileoverview Disallow `.js` extension in dependency paths
+ * @fileoverview Rule to disallow `.js` extension in dependency paths
  * @author Casey Visco
  */
 
 "use strict";
 
 const util = require("../util");
 
+const isDefineCall = util.isDefineCall;
+const isRequireCall = util.isRequireCall;
+const getDependencyStringNodes = util.getDependencyStringNodes;
+
+// -----------------------------------------------------------------------------
+// Rule Definition
+// -----------------------------------------------------------------------------
+
+const ERROR_MSG = "Don't use .js extension in dependency path.";
+
 module.exports = {
     meta: {
-        docs: {},
+        docs: {
+            description: "Disallow `.js` extension in dependency paths",
+            category: "Possible Errors",
+            recommended: true
+        },
         schema: [
             {
                 "type": "array",
@@ -22,52 +36,33 @@ module.exports = {
     },
 
     create: function (context) {
-        const MESSAGE = "Don't use .js extension in dependency path.";
-        const ALL_PLUGINS_PREFIX_REGEX = /^(\w+)\!/i;
-
         const pluginsToCheck = context.options[0] || [];
-        const listedPluginsPrefixRegex = new RegExp("^(" + pluginsToCheck.join("|") + ")\!", "i");
+        const LISTED_PLUGINS = new RegExp("^(" + pluginsToCheck.join("|") + ")\!", "i");
+        const ALL_PLUGINS = /^(\w+)\!/i;
 
         /**
-         * Determines if provided `node` should be checked for .js extension or not.
-         *
+         * Determines if provided `node` should be checked for .js extension or
+         * not. Path's to check either do not have a plugin prefix OR have a
+         * plugin prefix that's supplied as an option to the rule.
          * @private
-         * @param {ASTNode} node - Literal node to test
-         * @returns {Boolean} true if literal node should be checked for .js extension
+         * @param {String} path - path to test
+         * @returns {Boolean} true if path should be checked for .js extension
          */
-        function shouldBeChecked(node) {
-            const path = node.value.trim();
-            let result = true;
-
-            if (!path.match(listedPluginsPrefixRegex) && path.match(ALL_PLUGINS_PREFIX_REGEX)) {
-                result = false;
-            }
-
-            return result;
+        function shouldBeChecked(path) {
+            return path.match(LISTED_PLUGINS) || !path.match(ALL_PLUGINS);
         }
 
-        /**
-         * Determine if provided `node` contains a path with a .js extension.
-         *
-         * @private
-         * @param  {ASTNode} node - Literal node to test
-         * @returns {Boolean} true if literal value ends in ".js"
-         */
-        function hasJsExtension(node) {
+        function check(node) {
             const path = node.value.trim();
-            const startAt = path.length - 3;
-
-            return path.indexOf(".js", startAt) !== -1;
+            if (shouldBeChecked(path) && path.endsWith(".js")) {
+                context.report(node, ERROR_MSG);
+            }
         }
 
         return {
             "CallExpression": function (node) {
-                if (util.isDefineCall(node) || util.isRequireCall(node)) {
-                    util.getDependencyStringNodes(node).forEach(function (pathNode) {
-                        if (shouldBeChecked(pathNode) && hasJsExtension(pathNode)) {
-                            context.report(pathNode, MESSAGE);
-                        }
-                    });
+                if (isDefineCall(node) || isRequireCall(node)) {
+                    getDependencyStringNodes(node).forEach(check);
                 }
             }
         };

diff --git a/lib/util.js b/lib/util.js
@@ -289,7 +289,7 @@ function isValidRequire(node) {
  * @returns {Boolean} true if represents an AMD style require function
  */
 function isAmdRequire(node) {
-    const args = node.arguments || [];
+    const args = node.arguments;
     return args.length === 2 && isArrayExpr(args[0]) && isFunctionExpr(args[1]);
 }
 

diff --git a/tests/lib/rules/no-js-extension.js b/tests/lib/rules/no-js-extension.js
@@ -9,6 +9,22 @@ const testRule = require("../../rule-tester");
 const fixtures = require("../../fixtures");
 const rule = require("../../../lib/rules/no-js-extension");
 
+// -----------------------------------------------------------------------------
+// Fixtures
+// -----------------------------------------------------------------------------
+
+const NON_REQUIREJS_DEFINE_WITH_JS_EXT = `
+    module([
+        'aaa.js'
+    ], function () {
+        /* ... */
+    });
+`;
+
+// -----------------------------------------------------------------------------
+// Tests
+// -----------------------------------------------------------------------------
+
 const ERROR = {
     message: "Don't use .js extension in dependency path.",
     type: "Literal"
@@ -54,6 +70,7 @@ testRule("no-js-extension", rule, {
         fixtures.CONDITIONAL_NESTED_AMD_REQUIREJS,
         fixtures.CONDITIONAL_TERNARY_CJS_REQUIRE,
         fixtures.CONDITIONAL_TERNARY_CJS_REQUIREJS,
+        NON_REQUIREJS_DEFINE_WITH_JS_EXT,
 
         // plugins check