Adjusting on-watch rule to avoid false positives

README.md

@@ -115,7 +115,7 @@ These are rules designed to prevent you from making mistakes. They either prescr
  * [no-inline-template](docs/rules/no-inline-template.md) - disallow the use of inline templates
  * [no-run-logic](docs/rules/no-run-logic.md) - keep run functions clean and simple ([y171](https://github.com/johnpapa/angular-styleguide/blob/master/a1/README.md#style-y171))
  * [no-services](docs/rules/no-services.md) - disallow DI of specified services for other angular components (`$http` for controllers, filters and directives)
- * [on-watch](docs/rules/on-watch.md) - require `$on` and `$watch` deregistration callbacks to be saved in a variable
+ * [on-watch](docs/rules/on-watch.md) - require `$on` and `$watch` deregistration callbacks not to be ignored
  * [prefer-component](docs/rules/prefer-component.md) - 
 
 ### Deprecated Angular Features

docs/rules/on-watch.md

@@ -1,8 +1,9 @@
 <!-- WARNING: Generated documentation. Edit docs and examples in the rule and examples file ('rules/on-watch.js', 'examples/on-watch.js'). -->
 
-# on-watch - require `$on` and `$watch` deregistration callbacks to be saved in a variable
+# on-watch - require `$on` and `$watch` deregistration callbacks not to be ignored
 
-Watch and On methods on the scope object should be assigned to a variable, in order to be deleted in a $destroy event handler
+Deregistration functions returned by Watch and On methods on the scope object should not be ignored, in order to be deleted in a $destroy event handler.
+They should be assigned to a variable, returned from a function, put in an array or passed to a function as an argument.
 
 **Rule based on Angular 1.x**
 
@@ -15,7 +16,7 @@ The following patterns are considered problems;
     // invalid
     $rootScope.$on('event', function () {
         // ...
-    }); // error: The "$on" call should be assigned to a variable, in order to be destroyed during the $destroy event
+    }); // error: The deregistration function returned by "$on" call call should not be ignored
 
 The following patterns are **not** considered problems;
 
@@ -27,10 +28,28 @@ The following patterns are **not** considered problems;
     });
 
     // valid
-    var unregister = $rootScope.$on('event', function () {
+    var deregister = $rootScope.$on('event', function () {
         // ...
     });
 
+    // valid
+    function watchLocalVariable(callback) {
+        return $rootScope.$watch(function() {
+            return watchLocalVariable;
+        }, callback);
+    }
+
+    // valid
+    var deregisterFns = [
+        $rootScope.$on('event', eventHandler),
+        $rootScope.$watch('expression', watcherHandler)
+    ];
+
+    // valid
+    deregisterFns.push($rootScope.$on('event', function() {
+        // ...
+    }));
+
 ## Version
 
 This rule was introduced in eslint-plugin-angular 0.1.0

examples/on-watch.js

@@ -4,11 +4,29 @@ $scope.$on('event', function () {
 });
 
 // example - valid: true
-var unregister = $rootScope.$on('event', function () {
+var deregister = $rootScope.$on('event', function () {
     // ...
 });
 
-// example - valid: false, errorMessage: "The \"$on\" call should be assigned to a variable, in order to be destroyed during the $destroy event"
+// example - valid: true
+function watchLocalVariable(callback) {
+    return $rootScope.$watch(function() {
+        return watchLocalVariable;
+    }, callback);
+}
+
+// example - valid: true
+var deregisterFns = [
+    $rootScope.$on('event', eventHandler),
+    $rootScope.$watch('expression', watcherHandler)
+];
+
+// example - valid: true
+deregisterFns.push($rootScope.$on('event', function() {
+    // ...
+}));
+
+// example - valid: false, errorMessage: "The deregistration function returned by \"$on\" call call should not be ignored"
 $rootScope.$on('event', function () {
     // ...
 });

rules/on-watch.js

@@ -1,7 +1,8 @@
 /**
- * require `$on` and `$watch` deregistration callbacks to be saved in a variable
+ * require `$on` and `$watch` deregistration callbacks not to be ignored
  *
- * Watch and On methods on the scope object should be assigned to a variable, in order to be deleted in a $destroy event handler
+ * Deregistration functions returned by Watch and On methods on the scope object should not be ignored, in order to be deleted in a $destroy event handler.
+ * They should be assigned to a variable, returned from a function, put in an array or passed to a function as an argument.
  * @version 0.1.0
  * @category bestPractice
  * @sinceAngularVersion 1.x
@@ -17,17 +18,16 @@ module.exports = {
     },
     create: function(context) {
         function report(node, method) {
-            context.report(node, 'The "{{method}}" call should be assigned to a variable, in order to be destroyed during the $destroy event', {
+            context.report(node, 'The deregistration function returned by "{{method}}" call should not be ignored', {
                 method: method
             });
         }
 
         /**
          * Return true if the given node is a call expression calling a function
-         * named '$on' or '$watch' on an object named '$scope', '$rootScope' or
-         * 'scope'.
+         * named '$on' or '$watch' on an object named '$rootScope'.
          */
-        function isScopeOnOrWatch(node, scopes) {
+        function isRootScopeOnOrWatch(node) {
             if (node.type !== 'CallExpression') {
                 return false;
             }
@@ -52,7 +52,7 @@ module.exports = {
             var objectName = parentObject.name;
             var functionName = accessedFunction.name;
 
-            return scopes.indexOf(objectName) >= 0 && (functionName === '$on' ||
+            return objectName === '$rootScope' && (functionName === '$on' ||
                 functionName === '$watch');
         }
 
@@ -71,11 +71,12 @@ module.exports = {
         return {
 
             CallExpression: function(node) {
-                if (isScopeOnOrWatch(node, ['$rootScope']) && !isFirstArgDestroy(node)) {
+                if (isRootScopeOnOrWatch(node) && !isFirstArgDestroy(node)) {
                     if (node.parent.type !== 'VariableDeclarator' &&
                         node.parent.type !== 'AssignmentExpression' &&
-                        !(isScopeOnOrWatch(node.parent, ['$rootScope', '$scope', 'scope']) &&
-                            isFirstArgDestroy(node.parent))) {
+                        node.parent.type !== 'ReturnStatement' &&
+                        node.parent.type !== 'CallExpression' &&
+                        node.parent.type !== 'ArrayExpression') {
                         report(node, node.callee.property.name);
                     }
                 }

test/on-watch.js

@@ -35,13 +35,16 @@ eslintTester.run('on-watch', rule, {
 
         // false positive check
         '$on()',
+        'function watchSomething() {return $rootScope.$watch()}',
+        'var variable = [$rootScope.$on(), $rootScope.$watch()]',
+        'var variable = []; variable.push($rootScope.$watch());',
 
         // uncovered edgecase
         '$scope["$on"]()'
 
     ].concat(commonFalsePositives),
     invalid: [
-        {code: '$rootScope.$on()', errors: [{message: 'The "$on" call should be assigned to a variable, in order to be destroyed during the $destroy event'}]},
-        {code: '$rootScope.$watch()', errors: [{message: 'The "$watch" call should be assigned to a variable, in order to be destroyed during the $destroy event'}]}
+        {code: '$rootScope.$on()', errors: [{message: 'The deregistration function returned by "$on" call should not be ignored'}]},
+        {code: '$rootScope.$watch()', errors: [{message: 'The deregistration function returned by "$watch" call should not be ignored'}]}
     ]
 });