Rule no-el-assign (fixes #12)

ilyavolodin · Sep 28, 2014 · d4996f7 · d4996f7
1 parent 49036b7
commit d4996f7
Show file tree

Hide file tree

Showing 14 changed files with 205 additions and 73 deletions.
diff --git a/.eslintignore b/.eslintignore
@@ -1 +1,2 @@
 node_modules/**
+build/**
diff --git a/README.md b/README.md
@@ -82,4 +82,5 @@ If you are using custom models/view/collection bases you also have to specify ea
 * [events-on-top](docs/rules/events-on-top.md)
 * [model-defaults](docs/rules/model-defaults.md)
 * [no-constructor](docs/rules/no-constructor.md)
+* [no-el-assign](docs/rules/no-el-assign.md)
 * [no-native-jquery](docs/rules/no-native-jquery.md)
diff --git a/backbone-helper.js b/backbone-helper.js
@@ -1,69 +1,69 @@
 function isBackboneBase(node, settings) {
-	"use strict";
-	var prefixes = settings.Collection.concat(settings.Model, settings.View).map(function(item) {
-		return item.prefix;
-	});
-	return node.type === "CallExpression" &&
-		node.callee.type === "MemberExpression" &&
-		(
-			(node.callee.object.type === "MemberExpression" && prefixes.indexOf(node.callee.object.object.name) > -1) ||
-			(node.callee.object.type === "Identifier" && prefixes.indexOf(node.callee.object.name) > -1)
-		) &&
-		node.callee.property.name === "extend";
+    "use strict";
+    var prefixes = settings.Collection.concat(settings.Model, settings.View).map(function(item) {
+        return item.prefix;
+    });
+    return node.type === "CallExpression" &&
+        node.callee.type === "MemberExpression" &&
+        (
+            (node.callee.object.type === "MemberExpression" && prefixes.indexOf(node.callee.object.object.name) > -1) ||
+            (node.callee.object.type === "Identifier" && prefixes.indexOf(node.callee.object.name) > -1)
+        ) &&
+        node.callee.property.name === "extend";
 }
 
-function isBackboneModel(node, settings) {
-	"use strict";
-	settings = normalizeSettings(settings);
-	return isBackboneBase(node, settings) && checkForBackboneType(settings.Model, node.callee.object);
+function parseSettings(settings) {
+    "use strict";
+    return settings.map(function(setting) {
+        if (typeof setting === "object") {
+            return setting;
+        }
+        var splitValue = setting.split(".");
+        return splitValue.length > 1 ? { prefix: splitValue[0], postfix: splitValue[1] } : { prefix: splitValue[0] };
+    });
 }
 
-function isBackboneView(node, settings) {
-	"use strict";
-	settings = normalizeSettings(settings);
-	return isBackboneBase(node, settings) && checkForBackboneType(settings.View, node.callee.object);
+function normalizeSettings(settings) {
+    "use strict";
+    settings = settings || {};
+    settings.Collection = settings.Collection ? settings.Collection.concat("Backbone.Collection") : ["Backbone.Collection"];
+    settings.Collection = parseSettings(settings.Collection);
+    settings.Model = settings.Model ? settings.Model.concat("Backbone.Model") : ["Backbone.Model"];
+    settings.Model = parseSettings(settings.Model);
+    settings.View = settings.View ? settings.View.concat("Backbone.View") : ["Backbone.View"];
+    settings.View = parseSettings(settings.View);
+    return settings;
 }
 
-function isBackboneCollection(node, settings) {
-	"use strict";
-	settings = normalizeSettings(settings);
-	return isBackboneBase(node, settings) && checkForBackboneType(settings.Collection, node.callee.object);
+function checkForBackboneType(settings, object) {
+    "use strict";
+    return settings.some(function(item) {
+        return item.postfix ? item.postfix === object.property.name : item.prefix === object.name;
+    });
 }
 
-function isBackboneAny(node, settings) {
-	"use strict";
-	settings = normalizeSettings(settings);
-	return isBackboneBase(node, settings) && node.callee && node.callee.object && (checkForBackboneType(settings.Model, node.callee.object) || checkForBackboneType(settings.View, node.callee.object) || checkForBackboneType(settings.Collection, node.callee.object));
+function isBackboneModel(node, settings) {
+    "use strict";
+    settings = normalizeSettings(settings);
+    return isBackboneBase(node, settings) && checkForBackboneType(settings.Model, node.callee.object);
 }
 
-function checkForBackboneType(settings, object) {
-	"use strict";
-	return settings.some(function(item) { 
-		return item.postfix ? item.postfix === object.property.name : item.prefix === object.name;
-	});
+function isBackboneView(node, settings) {
+    "use strict";
+    settings = normalizeSettings(settings);
+    return isBackboneBase(node, settings) && checkForBackboneType(settings.View, node.callee.object);
 }
 
-function normalizeSettings(settings) {
-	"use strict";
-	settings = settings || {};
-	settings.Collection = settings.Collection ? settings.Collection.concat('Backbone.Collection') : ['Backbone.Collection'];
-	settings.Collection = parseSettings(settings.Collection);
-	settings.Model = settings.Model ? settings.Model.concat('Backbone.Model') : ['Backbone.Model'];
-	settings.Model = parseSettings(settings.Model);
-	settings.View = settings.View ? settings.View.concat('Backbone.View') : ['Backbone.View'];
-	settings.View = parseSettings(settings.View);
-	return settings;
+function isBackboneCollection(node, settings) {
+    "use strict";
+    settings = normalizeSettings(settings);
+    return isBackboneBase(node, settings) && checkForBackboneType(settings.Collection, node.callee.object);
 }
 
-function parseSettings(settings) {
-	"use strict";
-	return settings.map(function(setting) {
-		if (typeof setting === "object") {
-			return setting;
-		}
-		var splitValue = setting.split('.');
-		return splitValue.length > 1 ? { prefix: splitValue[0], postfix: splitValue[1] } : { prefix: splitValue[0] };
-	});
+function isBackboneAny(node, settings) {
+    "use strict";
+    settings = normalizeSettings(settings);
+    return isBackboneBase(node, settings) && node.callee && node.callee.object && (checkForBackboneType(settings.Model, node.callee.object) || checkForBackboneType(settings.View, node.callee.object) || checkForBackboneType(settings.Collection, node.callee.object));
 }
 
 exports.isBackboneAny = isBackboneAny;
@@ -72,25 +72,25 @@ exports.isBackboneView = isBackboneView;
 exports.isBackboneCollection = isBackboneCollection;
 
 exports.checkIfPropertyInBackbone = function(node, settings) {
-	"use strict";
-	var parent = node.parent, grandparent = parent.parent, greatgrandparent = grandparent.parent;
-	return isBackboneAny(greatgrandparent, settings);
+    "use strict";
+    var parent = node.parent, grandparent = parent.parent, greatgrandparent = grandparent.parent;
+    return isBackboneAny(greatgrandparent, settings);
 };
 
 exports.checkIfPropertyInBackboneModel = function(node, settings) {
-	"use strict";
-	var parent = node.parent, grandparent = parent.parent, greatgrandparent = grandparent.parent;
-	return isBackboneModel(greatgrandparent, settings);
+    "use strict";
+    var parent = node.parent, grandparent = parent.parent, greatgrandparent = grandparent.parent;
+    return isBackboneModel(greatgrandparent, settings);
 };
 
 exports.checkIfPropertyInBackboneView = function(node, settings) {
-	"use strict";
-	var parent = node.parent, grandparent = parent.parent, greatgrandparent = grandparent.parent;
-	return isBackboneView(greatgrandparent, settings);
+    "use strict";
+    var parent = node.parent, grandparent = parent.parent, greatgrandparent = grandparent.parent;
+    return isBackboneView(greatgrandparent, settings);
 };
 
 exports.checkIfPropertyInBackboneCollection = function(node, settings) {
-	"use strict";
-	var parent = node.parent, grandparent = parent.parent, greatgrandparent = grandparent.parent;
-	return isBackboneCollection(greatgrandparent, settings);
+    "use strict";
+    var parent = node.parent, grandparent = parent.parent, greatgrandparent = grandparent.parent;
+    return isBackboneCollection(greatgrandparent, settings);
 };
diff --git a/docs/rules/no-el-assign.md b/docs/rules/no-el-assign.md
@@ -0,0 +1,43 @@
+# Prevent assigning el or $el inside views (no-el-assign)
+
+Settings `this.el` or `this.$el` can lead to issues, since Backbone will not automatically bind events to the element. Use `setElement` function instead which will cache a reference to an element and bind all events.
+
+## Rule Details
+
+The following patterns are considered warnings:
+
+```js
+
+Backbone.View.extend({
+    initialize: function() {
+        this.$el = $(".test");
+    }
+});
+
+Backbone.View.extend({
+    initialize: function() {
+        this.el = $(".test")[0];
+    }
+});
+
+```
+
+The following patterns are not warnings:
+
+```js
+
+Backbone.View.extend({
+    initialize: function() {
+        this.setElement($(".test"));
+    }
+});
+
+```
+
+## When Not To Use It
+
+If you always call `delegateEvents` manually, you can still set this.el directly.
+
+## Further Reading
+
+[BackboneJS Documentation](http://backbonejs.org/#View-setElement)
diff --git a/index.js b/index.js
@@ -7,6 +7,7 @@ module.exports = {
         "events-on-top": require("./lib/rules/events-on-top"),
         "model-defaults": require("./lib/rules/model-defaults"),
         "no-constructor": require("./lib/rules/no-constructor"),
+        "no-el-assign": require("./lib/rules/no-el-assign"),
         "no-native-jquery": require("./lib/rules/no-native-jquery")
     }
 };
diff --git a/lib/rules/collection-model.js b/lib/rules/collection-model.js
@@ -12,6 +12,7 @@ var helper = require("../../backbone-helper.js");
 
 module.exports = function(context) {
 
+    var settings = context.settings || /* istanbul ignore next */ {};
     var backboneCollection = [];
 
     //--------------------------------------------------------------------------
@@ -20,7 +21,7 @@ module.exports = function(context) {
 
     return {
         "CallExpression": function(node) {
-            if (helper.isBackboneCollection(node, context.settings.backbone)) {
+            if (helper.isBackboneCollection(node, settings.backbone)) {
                 backboneCollection.push({ node: node, found: false });
             }
         },
@@ -35,7 +36,7 @@ module.exports = function(context) {
         },
         "Identifier": function(node) {
             if (backboneCollection.length > 0 && node.name === "model") {
-                if (helper.checkIfPropertyInBackboneCollection(node, context.settings.backbone)) {
+                if (helper.checkIfPropertyInBackboneCollection(node, settings.backbone)) {
                     backboneCollection[backboneCollection.length - 1].found = true;
                 }
             }

diff --git a/lib/rules/defaults-on-top.js b/lib/rules/defaults-on-top.js
@@ -12,6 +12,8 @@ var helper = require("../../backbone-helper.js");
 
 module.exports = function(context) {
 
+    var settings = context.settings || /* istanbul ignore next */ {};
+
     //--------------------------------------------------------------------------
     // Public
     //--------------------------------------------------------------------------
@@ -20,7 +22,7 @@ module.exports = function(context) {
         "Identifier": function(node) {
             if (node.name === "defaults") {
                 var parent = node.parent, grandparent = parent.parent;
-                if (helper.checkIfPropertyInBackboneModel(node, context.settings.backbone) && grandparent.type === "ObjectExpression" &&  grandparent.properties[0] !== parent) {
+                if (helper.checkIfPropertyInBackboneModel(node, settings.backbone) && grandparent.type === "ObjectExpression" &&  grandparent.properties[0] !== parent) {
                     context.report(node, "defaults should be declared at the top of the model.");
                 }
             }

diff --git a/lib/rules/events-on-top.js b/lib/rules/events-on-top.js
@@ -12,6 +12,8 @@ var helper = require("../../backbone-helper.js");
 
 module.exports = function(context) {
 
+    var settings = context.settings || /* istanbul ignore next */ {};
+
     //--------------------------------------------------------------------------
     // Public
     //--------------------------------------------------------------------------
@@ -20,7 +22,7 @@ module.exports = function(context) {
         "Identifier": function(node) {
             if (node.name === "events") {
                 var parent = node.parent, grandparent = parent.parent;
-                if (helper.checkIfPropertyInBackboneView(node, context.settings.backbone) && grandparent.type === "ObjectExpression" &&  grandparent.properties[0] !== parent) {
+                if (helper.checkIfPropertyInBackboneView(node, settings.backbone) && grandparent.type === "ObjectExpression" &&  grandparent.properties[0] !== parent) {
                     context.report(node, "events should be declared at the top of the view.");
                 }
             }

diff --git a/lib/rules/model-defaults.js b/lib/rules/model-defaults.js
@@ -13,14 +13,15 @@ var helper = require("../../backbone-helper.js");
 module.exports = function(context) {
 
     var backboneModel = [];
+    var settings = context.settings || /* istanbul ignore next */ {};
 
     //--------------------------------------------------------------------------
     // Public
     //--------------------------------------------------------------------------
 
     return {
         "CallExpression": function(node) {
-            if (helper.isBackboneModel(node, context.settings.backbone)) {
+            if (helper.isBackboneModel(node, settings.backbone)) {
                 backboneModel.push({ node: node, found: false });
             }
         },
@@ -35,7 +36,7 @@ module.exports = function(context) {
         },
         "Identifier": function(node) {
             if (backboneModel.length > 0 && node.name === "defaults") {
-                if (helper.checkIfPropertyInBackboneModel(node, context.settings.backbone)) {
+                if (helper.checkIfPropertyInBackboneModel(node, settings.backbone)) {
                     backboneModel[backboneModel.length - 1].found = true;
                 }
             }

diff --git a/lib/rules/no-constructor.js b/lib/rules/no-constructor.js
@@ -12,14 +12,16 @@ var helper = require("../../backbone-helper.js");
 
 module.exports = function(context) {
 
+    var settings = context.settings || /* istanbul ignore next */ {};
+
     //--------------------------------------------------------------------------
     // Public
     //--------------------------------------------------------------------------
 
     return {
         "Identifier": function(node) {
             if (node.name === "constructor") {
-                if (helper.checkIfPropertyInBackbone(node, context.settings.backbone)) {
+                if (helper.checkIfPropertyInBackbone(node, settings.backbone)) {
                     context.report(node, "Overload initialize instead of constructor");
                 }
             }

diff --git a/lib/rules/no-el-assign.js b/lib/rules/no-el-assign.js
@@ -0,0 +1,41 @@
+/**
+ * @fileoverview Prevent assigning el or $el inside views
+ * @author Ilya Volodin
+ */
+"use strict";
+
+//------------------------------------------------------------------------------
+// Rule Definition
+//------------------------------------------------------------------------------
+
+var helper = require("../../backbone-helper.js");
+
+module.exports = function(context) {
+
+    var backboneView = [];
+    var settings = context.settings || /* istanbul ignore next */ {};
+
+    //--------------------------------------------------------------------------
+    // Public
+    //--------------------------------------------------------------------------
+
+    return {
+        "CallExpression": function(node) {
+            backboneView.push(backboneView[backboneView.length - 1] || helper.isBackboneView(node, settings.backbone));
+        },
+        "CallExpression:exit": function(node) {
+            if (helper.isBackboneView(node, settings.backbone)) {
+                backboneView.pop();
+            }
+        },
+        "AssignmentExpression" : function(node) {
+            if (backboneView[backboneView.length - 1] &&
+                node.left.type === "MemberExpression" &&
+                node.left.object.type === "ThisExpression" &&
+                (node.left.property.name === "el" || node.left.property.name === "$el")) {
+                    context.report(node, "Do not assign '{{identifier}}' directly. Use setElement() instead", { identifier: node.left.property.name });
+            }
+        }
+    };
+
+};
diff --git a/lib/rules/no-native-jquery.js b/lib/rules/no-native-jquery.js
@@ -13,17 +13,18 @@ var helper = require("../../backbone-helper.js");
 module.exports = function(context) {
 
     var backboneView = [];
+    var settings = context.settings || /* istanbul ignore next */ {};
 
     //--------------------------------------------------------------------------
     // Public
     //--------------------------------------------------------------------------
 
     return {
         "CallExpression": function(node) {
-            backboneView.push(backboneView[backboneView.length - 1] || helper.isBackboneView(node, context.settings.backbone));
+            backboneView.push(backboneView[backboneView.length - 1] || helper.isBackboneView(node, settings.backbone));
         },
         "CallExpression:exit": function(node) {
-            if (helper.isBackboneView(node, context.settings.backbone)) {
+            if (helper.isBackboneView(node, settings.backbone)) {
                 backboneView.pop();
             }
         },

diff --git a/package.json b/package.json
@@ -5,7 +5,7 @@
     "main": "index.js",
     "scripts": {
         "test": "npm run lint && npm run unit-test --coverage && npm run check-coverage",
-        "lint": "eslint ./lib",
+        "lint": "eslint ./",
         "unit-test": "istanbul test --dir build/coverage node_modules/mocha/bin/_mocha tests/**/*.js -- --recursive --reporter dot",
         "check-coverage": "istanbul check-coverage --statement 100 --branch 100 --function 100 --lines 100",
         "report-coverage-html": "istanbul report --dir  build/coverage html"