New: Implement no-commonjs-return rule (fixes #4)

* Add more appropriate return values to test fixtures

README.md

@@ -53,6 +53,7 @@ Enable the rules that you would like to use, for example:
 * [no-named-define](docs/rules/no-named-define.md): Disallow named module form of `define` (off by default)
 * [no-commonjs-wrapper](docs/rules/no-commonjs-wrapper.md): Disallow use of Simplified CommonJS Wrapper (off by default)
 * [no-assign-exports](docs/rules/no-assign-exports.md): Disallow assignment to `exports` when using Simplified CommonJS Wrapper
+* [no-commonjs-return](docs/rules/no-commonjs-return.md): Disallow use of `return` statement in a module definition when using Simplified CommonJS Wrapper (off by default)
 
 ### Don't see the rule you're looking for?
 

docs/rules/no-commonjs-return.md

@@ -0,0 +1,49 @@
+# Disallow use of `return` statement in a module definition when using Simplified CommonJS Wrapper (no-commonjs-return)
+
+When using the Simplified CommonJS Wrapper, module definition functions may export a value in one of three ways; by assigning to `module.exports`, adding a property to the `exports` object, or by directly returning a value. Exporting a value via `return` is typical of AMD modules, but not CommonJS modules. If you're using the Simplified CommonJS Wrapper, you may wish to enable this rule.
+
+Note that this rule is only in affect when using the Simplified CommonJS Wrapper form of `define`. It will *not* flag
+`return` statements in AMD definitions or simple function definitions.
+
+## Rule Details
+
+This rule aims to prevent exporting module values with `return`.
+
+The following patterns are considered warnings:
+
+```js
+define(function (require) {
+    var foo = require('path/to/foo'),
+        bar = require('path/to/bar');
+
+    return function () { /* ... */ };
+});
+```
+
+The following patterns are not warnings:
+
+```js
+define(function (require, exports) {
+    var foo = require('path/to/foo'),
+        bar = require('path/to/bar');
+
+    exports.doSomething = function () { /* ... */ };
+});
+
+define(function (require, exports, module) {
+    var foo = require('path/to/foo'),
+        bar = require('path/to/bar');
+
+    module.exports = {
+        doSomething: function () { /* ... */ };
+    }
+});
+```
+
+## When Not To Use It
+
+If you want to allow exporting a module value using the `return` statement, then it is safe to disable this rule.
+
+## Further Reading
+
+* [Define a Module with Simplified CommonJS Wrapper](http://requirejs.org/docs/api.html#cjsmodule)

index.js

@@ -4,12 +4,13 @@ module.exports = {
     rules: {
         "no-invalid-define": require("./lib/rules/no-invalid-define"),
         "no-multiple-define": require("./lib/rules/no-multiple-define"),
+        "no-assign-exports": require("./lib/rules/no-assign-exports"),
         "no-object-define": require("./lib/rules/no-object-define"),
         "no-function-define": require("./lib/rules/no-function-define"),
         "no-amd-define": require("./lib/rules/no-amd-define"),
         "no-named-define": require("./lib/rules/no-named-define"),
         "no-commonjs-wrapper": require("./lib/rules/no-commonjs-wrapper"),
-        "no-assign-exports": require("./lib/rules/no-assign-exports")
+        "no-commonjs-return": require("./lib/rules/no-commonjs-return")
     },
     rulesConfig: {
 
@@ -23,6 +24,7 @@ module.exports = {
         "no-function-define": 0,
         "no-amd-define": 0,
         "no-named-define": 0,
-        "no-commonjs-wrapper": 0
+        "no-commonjs-wrapper": 0,
+        "no-commonjs-return": 0
     }
 };

lib/rules/no-commonjs-return.js

@@ -0,0 +1,52 @@
+/**
+ * @fileoverview Disallow use of `return` statement in a module definition when using Simplified CommonJS Wrapper
+ * @author Casey Visco
+ */
+
+"use strict";
+
+//------------------------------------------------------------------------------
+// Requirements
+//------------------------------------------------------------------------------
+
+var helpers = require("../helpers");
+
+
+//------------------------------------------------------------------------------
+// Rule Definition
+//------------------------------------------------------------------------------
+
+module.exports = function (context) {
+
+    var MESSAGE = "Unexpected `return` in module definition. Use `exports` or `module.exports` instead.";
+    var functions = [];
+
+    //--------------------------------------------------------------------------
+    // Public
+    //--------------------------------------------------------------------------
+
+    return {
+        "FunctionExpression": function (node) {
+            var parent = node.parent,
+                isDefineCall = parent.type === "CallExpression" && parent.callee.name === "define";
+
+            functions.push({
+                isModuleDef: isDefineCall && helpers.isCommonJsDef(parent.arguments)
+            });
+        },
+
+        "FunctionExpression:exit": function () {
+            functions.pop();
+        },
+
+        "ReturnStatement": function (node) {
+            var currentFunction = functions[functions.length - 1];
+
+            if (currentFunction.isModuleDef) {
+                context.report(node, MESSAGE);
+            }
+        }
+    };
+
+};
+

tests/lib/fixtures.js

@@ -6,19 +6,19 @@ module.exports = {
         // Always Invalid
         EMPTY: "define();",
         NONSENSE: "define('foo', 'bar', false);",
-        MULTIPLE: "define({ foo: 'bar' }); define(function () { return true; })",
+        MULTIPLE: "define({ foo: 'bar' }); define(function () { return { foo: 'bar' }; })",
 
         // Valid unless disabled
         OBJECT: "define({ a: 'foo', b: 'bar' });",
-        FUNCTION: "define(function () { return true; });",
-        COMMONJS_1: "define(function (require) { var a = require('path/to/a'), b = require('path/to/b'); return true; });",
-        COMMONJS_2: "define(function (require, exports) { var a = require('path/to/a'), b = require('path/to/b'); return true; });",
-        COMMONJS_3: "define(function (require, exports, module) { var a = require('path/to/a'), b = require('path/to/b'); return true; });",
-        AMD: "define(['path/to/a', 'path/to/b'], function (a, b) { return true; });",
-        AMD_EMPTY: "define([], function () { return true; });",
-        AMD_NAMED: "define('path/to/c', ['path/to/a', 'path/to/b'], function (a, b) { return true; });",
-        AMD_NAMED_EMPTY: "define('path/to/c', [], function () { return true; });"
-
+        FUNCTION: "define(function () { return { foo: 'bar' }; });",
+        AMD: "define(['path/to/a', 'path/to/b'], function (a, b) { return { foo: 'bar' }; });",
+        AMD_EMPTY: "define([], function () { return { foo: 'bar' }; });",
+        AMD_NAMED: "define('path/to/c', ['path/to/a', 'path/to/b'], function (a, b) { return { foo: 'bar' }; });",
+        AMD_NAMED_EMPTY: "define('path/to/c', [], function () { return { foo: 'bar' }; });",
+        COMMONJS_1: "define(function (require) { var a = require('path/to/a'), b = require('path/to/b'); return { foo: 'bar' }; });",
+        COMMONJS_2: "define(function (require, exports) { var a = require('path/to/a'), b = require('path/to/b'); exports.foo = 'bar'; });",
+        COMMONJS_3: "define(function (require, exports, module) { var a = require('path/to/a'), b = require('path/to/b'); module.exports = { foo: 'bar' }; });",
+        CJS_WITH_FUNC_EXPR: "define(function (require, exports, module) { var f = function () { return 'foo'; }; module.exports = { foo: f }; });"
     },
 
     exports: {

tests/lib/rules/no-commonjs-return.js

@@ -0,0 +1,47 @@
+/**
+ * @fileoverview Tests for `no-commonjs-return` rule
+ * @author Casey Visco <cvisco@gmail.com>
+ */
+
+"use strict";
+
+//------------------------------------------------------------------------------
+// Requirements
+//------------------------------------------------------------------------------
+
+var eslint = require("eslint"),
+    ESLintTester = require("eslint-tester"),
+    fixtures = require("../fixtures");
+
+
+//------------------------------------------------------------------------------
+// Tests
+//------------------------------------------------------------------------------
+
+var eslintTester = new ESLintTester(eslint.linter);
+
+eslintTester.addRuleTest("lib/rules/no-commonjs-return", {
+
+    valid: [
+        fixtures.define.OBJECT,
+        fixtures.define.FUNCTION,
+        fixtures.define.AMD,
+        fixtures.define.AMD_EMPTY,
+        fixtures.define.AMD_NAMED,
+        fixtures.define.AMD_NAMED_EMPTY,
+        fixtures.define.COMMONJS_2,
+        fixtures.define.COMMONJS_3,
+        fixtures.define.CJS_WITH_FUNC_EXPR
+    ],
+
+    invalid: [
+        {
+            code: fixtures.define.COMMONJS_1,
+            errors: [{
+                message: "Unexpected `return` in module definition. Use `exports` or `module.exports` instead.",
+                type: "ReturnStatement"
+            }]
+        }
+    ]
+
+});