From 534740c9273e396d6359aef35f78afdb0097de82 Mon Sep 17 00:00:00 2001 From: Nik Karbaum Date: Fri, 13 Nov 2015 22:43:30 +0100 Subject: [PATCH] Improve "on" lint behaviour * The lint will from now on only handle "$on" calls, as "$watches" are automatically removed upon destroying the scope. * The lint will no longer complain about not unsubscribing from the $rootScope. While there can be cases where this is valid, most of the time it isn't * Adapt tests * Adapt documentation --- README.md | 4 ++-- index.js | 2 +- rules/{on-watch.js => on.js} | 32 +++++++++++++------------ test/on-watch.js | 46 ------------------------------------ test/on.js | 32 +++++++++++++++++++++++++ 5 files changed, 52 insertions(+), 64 deletions(-) rename rules/{on-watch.js => on.js} (63%) delete mode 100644 test/on-watch.js create mode 100644 test/on.js diff --git a/README.md b/README.md index 07f0ce9d..c63cc935 100644 --- a/README.md +++ b/README.md @@ -132,7 +132,7 @@ Users may use the shareable [eslint-config-angular](https://github.com/dustinspe "angular/no-private-call": 2, "angular/no-service-method": 2, "angular/no-services": [2, ["$http", "$resource", "Restangular"]], - "angular/on-watch": 2, + "angular/on": 2, "angular/rest-service": 0, "angular/service-name": 2, "angular/timeout-service": 2, @@ -191,7 +191,7 @@ Users may use the shareable [eslint-config-angular](https://github.com/dustinspe | no-private-call | All scope's properties/methods starting with $$ are used internally by AngularJS. You should not use them directly. Exception can be allowed with this option: {allow:['$$watchers']} | | no-service-method | You should prefer the factory() method instead of service() [Y040](https://github.com/johnpapa/angular-styleguide#style-y040)| | no-services | Some services should be used only in a specific AngularJS service (Ajax-based service for example), in order to follow the separation of concerns paradigm. The second parameter specifies the services. The third parameter can be a list of angular objects (controller, factory, etc.). Or second parameter can be an object, where keys are angular object names and value is a list of services (like {controller: ['$http'], factory: ['$q']}) | -| on-watch | Watch and On methods on the scope object should be assigned to a variable, in order to be deleted in a $destroy event handler | +| on | "On" methods on the scope object should be assigned to a variable, in order to be deleted in a $destroy event handler | | rest-service | Check the service used to send request to your REST API. This rule can have one parameter, with one of the following values: $http, $resource or Restangular ('rest-service': [0, '$http']). | | service-name | All your services should have a name starting with the parameter you can define in your config object. The second parameter can be a Regexp wrapped in quotes. You can not prefix your services by "$" (reserved keyword for AngularJS services) ("service-name": [2, "ng"]) [Y125](https://github.com/johnpapa/angular-styleguide#style-y125) | | timeout-service | Instead of the default setTimeout function, you should use the AngularJS wrapper service $timeout [Y181](https://github.com/johnpapa/angular-styleguide#style-y181) | diff --git a/index.js b/index.js index 0f8717d2..5a0f9279 100644 --- a/index.js +++ b/index.js @@ -38,7 +38,7 @@ rulesConfiguration.addRule('no-jquery-angularelement', 2); rulesConfiguration.addRule('no-private-call', 2); rulesConfiguration.addRule('no-services', [2, ['$http', '$resource', 'Restangular', '$q']]); rulesConfiguration.addRule('no-service-method', 2); -rulesConfiguration.addRule('on-watch', 2); +rulesConfiguration.addRule('on', 2); rulesConfiguration.addRule('rest-service', 0); rulesConfiguration.addRule('service-name', 0); rulesConfiguration.addRule('timeout-service', 2); diff --git a/rules/on-watch.js b/rules/on.js similarity index 63% rename from rules/on-watch.js rename to rules/on.js index ddb8a519..9dcfef64 100644 --- a/rules/on-watch.js +++ b/rules/on.js @@ -9,10 +9,9 @@ module.exports = function(context) { /** * 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' on an object named '$scope' or 'scope'. */ - function isScopeOnOrWatch(node, scopes) { + function isScopeOn(node) { if (node.type !== 'CallExpression') { return false; } @@ -37,18 +36,18 @@ module.exports = function(context) { var objectName = parentObject.name; var functionName = accessedFunction.name; - return scopes.indexOf(objectName) >= 0 && (functionName === '$on' || - functionName === '$watch'); + return ['$scope', 'scope'].indexOf(objectName) >= 0 && functionName === '$on'; } /** - * Return true if the given node is a call expression that has a first + * Return true if the given node or its parent is a call expression that has a first * argument of the string '$destroy'. */ function isFirstArgDestroy(node) { - var args = node.arguments; + var args = (node.arguments || []).length > 0 ? node.arguments : node.parent.arguments; - return (args.length >= 1 && + return args && + (args.length >= 1 && args[0].type === 'Literal' && args[0].value === '$destroy'); } @@ -56,13 +55,16 @@ module.exports = function(context) { return { CallExpression: function(node) { - if (isScopeOnOrWatch(node, ['$rootScope']) && !isFirstArgDestroy(node)) { - if (node.parent.type !== 'VariableDeclarator' && - node.parent.type !== 'AssignmentExpression' && - !(isScopeOnOrWatch(node.parent, ['$rootScope', '$scope', 'scope']) && - isFirstArgDestroy(node.parent))) { - report(node, node.callee.property.name); - } + if (!isScopeOn(node)) { + return; + } + + if (isFirstArgDestroy(node)) { + return; + } + + if (node.parent.type !== 'VariableDeclarator' && node.parent.type !== 'AssignmentExpression') { + report(node, node.callee.property.name); } } }; diff --git a/test/on-watch.js b/test/on-watch.js deleted file mode 100644 index bfc2d623..00000000 --- a/test/on-watch.js +++ /dev/null @@ -1,46 +0,0 @@ -'use strict'; - -// ------------------------------------------------------------------------------ -// Requirements -// ------------------------------------------------------------------------------ - -var rule = require('../rules/on-watch'); -var RuleTester = require('eslint').RuleTester; - -// ------------------------------------------------------------------------------ -// Tests -// ------------------------------------------------------------------------------ - -var eslintTester = new RuleTester(); - -eslintTester.run('on-watch', rule, { - valid: [ - 'var variable = scope.$on()', - 'var variable = scope.$watch()', - 'var variable = $scope.$on()', - 'var variable = $scope.$watch()', - 'var variable = $rootScope.$on()', - 'var variable = $rootScope.$watch()', - '$scope.$on("$destroy")', - '$rootScope.$on("$destroy")', - '$scope.$on("$destroy", $scope.$on())', - '$rootScope.$on("$destroy", $scope.$on())', - '$scope.$on("$destroy", $rootScope.$on())', - '$rootScope.$on("$destroy", $rootScope.$on())', - 'scope.$on()', - 'scope.$watch()', - '$scope.$on()', - '$scope.$watch()', - - // false positive check - '$on()', - - // uncovered edgecase - '$scope["$on"]()' - - ], - 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'}]} - ] -}); diff --git a/test/on.js b/test/on.js new file mode 100644 index 00000000..99e033ef --- /dev/null +++ b/test/on.js @@ -0,0 +1,32 @@ +'use strict'; + +// ------------------------------------------------------------------------------ +// Requirements +// ------------------------------------------------------------------------------ + +var rule = require('../rules/on'); +var RuleTester = require('eslint').RuleTester; + +// ------------------------------------------------------------------------------ +// Tests +// ------------------------------------------------------------------------------ + +var eslintTester = new RuleTester(); + +eslintTester.run('on', rule, { + valid: [ + 'var variable = scope.$on()', + 'var variable = $scope.$on()', + '$scope.$on("$destroy")', + '$scope.$on("$destroy", $scope.$on())', + '$scope.$on("$destroy", $rootScope.$on())', + + // false positive check + '$on()' + + ], + invalid: [ + {code: '$scope.$on()', errors: [{message: 'The "$on" call should be assigned to a variable, in order to be destroyed during the $destroy event'}]}, + {code: 'scope.$on()', errors: [{message: 'The "$on" call should be assigned to a variable, in order to be destroyed during the $destroy event'}]} + ] +});