diff --git a/README.md b/README.md index 6e68c9e9..957be38a 100644 --- a/README.md +++ b/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 diff --git a/docs/rules/on-watch.md b/docs/rules/on-watch.md index 80406a48..af378f70 100644 --- a/docs/rules/on-watch.md +++ b/docs/rules/on-watch.md @@ -1,8 +1,9 @@ -# 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 diff --git a/examples/on-watch.js b/examples/on-watch.js index 7d050583..35b35c86 100644 --- a/examples/on-watch.js +++ b/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 () { // ... }); diff --git a/rules/on-watch.js b/rules/on-watch.js index e6d04603..6b2a8c44 100644 --- a/rules/on-watch.js +++ b/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); } } diff --git a/test/on-watch.js b/test/on-watch.js index ab8e7816..efb4b049 100644 --- a/test/on-watch.js +++ b/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'}]} ] });