From 24ebede738b36f30bab2208672ab602006d82015 Mon Sep 17 00:00:00 2001 From: Rajasegar Chandran Date: Sat, 25 May 2019 17:49:31 +0530 Subject: [PATCH 1/7] [NEW RULE] Adding no-async-actions rule Fixes #418 [DO NOT MERGE] 1. Working for normal async actions 2. With decorators it is throwing errors, need to fix this. --- lib/rules/no-async-actions.js | 61 ++++++++++++++++++ tests/lib/rules/no-async-actions.js | 96 +++++++++++++++++++++++++++++ 2 files changed, 157 insertions(+) create mode 100644 lib/rules/no-async-actions.js create mode 100644 tests/lib/rules/no-async-actions.js diff --git a/lib/rules/no-async-actions.js b/lib/rules/no-async-actions.js new file mode 100644 index 0000000000..e1f29c5dad --- /dev/null +++ b/lib/rules/no-async-actions.js @@ -0,0 +1,61 @@ +'use strict'; + +const utils = require('../utils/utils'); + + +//------------------------------------------------------------------------------ +// General rule - Don't use async actions +//------------------------------------------------------------------------------ + + +const message = 'Do not use async actions'; + +module.exports = { + meta: { + docs: { + description: 'Disallow usage of async actions in components', + category: 'Possible Errors', + recommended: true, + url: 'http://ember-concurrency.com/docs/tutorial' + }, + fixable: null, + }, + + create(context) { + return { + Property(node) { + if (node.key.name === 'actions') { + const props = node.value.properties; + + props.forEach((p) => { + const body = p.value.body.body; + if (p.value.async) { + context.report({ + node: p, + message, + }); + } else if (body.length === 1 && utils.isReturnStatement(body[0])) { + const retSt = body[0]; + if (retSt.argument.type === 'CallExpression' && + retSt.argument.callee.property.name === 'then') { + context.report({ + node: retSt, + message, + }); + } + } + }); + } else if (node.decorators) { + if (node.decorators.find(d => d.expression.name === 'action')) { + if (node.value.async) { + context.report({ + node, + message: 'Do not use async actions' + }); + } + } + } + } + }; + } +}; diff --git a/tests/lib/rules/no-async-actions.js b/tests/lib/rules/no-async-actions.js new file mode 100644 index 0000000000..5269bec87e --- /dev/null +++ b/tests/lib/rules/no-async-actions.js @@ -0,0 +1,96 @@ +'use strict'; + +//------------------------------------------------------------------------------ +// Requirements +//------------------------------------------------------------------------------ + +const rule = require('../../../lib/rules/no-async-actions.js'); +const RuleTester = require('eslint').RuleTester; + + +//------------------------------------------------------------------------------ +// Tests +//------------------------------------------------------------------------------ + +const message = 'Do not use async actions'; + +const ruleTester = new RuleTester({ + parserOptions: { + ecmaVersion: 2018, + sourceType: 'module', + } +}); + +ruleTester.run('no-async-actions', rule, { + valid: [ + { + code: ` + Component.extend({ + actions: { + handleClick() { + // ... + } + } + });`, + } + + ], + + invalid: [ + { + code: `Component.extend({ + actions: { + async handleClick() { + // ... + } + } + });`, + output: null, + errors: [{ + message, + }] + }, + { + code: `Component.extend({ + actions: { + handleClick() { + return something.then(() => { + let hello = "world"; + }); + } + } + });`, + output: null, + errors: [{ + message, + }] + }, + { + code: `Component.extend({ + @action + async handleClick() { + // ... + } + });`, + output: null, + errors: [{ + message, + }] + }, + { + code: `Component.extend({ + @action + handleClick() { + return something.then(() => { + let hello = "world"; + }); + } + });`, + output: null, + errors: [{ + message, + }] + }, + + ] +}); From 84ffc97102d657ad0ba2c4fa128f2cf1b1ef8bf4 Mon Sep 17 00:00:00 2001 From: Rajasegar Chandran Date: Wed, 29 May 2019 06:30:49 +0530 Subject: [PATCH 2/7] [DOCS] Adding doc pages for the rule 1. Also adding the rule to the Best practices section in README --- README.md | 1 + docs/rules/no-async-actions.md | 50 ++++++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+) create mode 100644 docs/rules/no-async-actions.md diff --git a/README.md b/README.md index 3068815aff..8fe774a04d 100644 --- a/README.md +++ b/README.md @@ -103,6 +103,7 @@ The `--fix` option on the command line automatically fixes problems reported by | :wrench: | [no-unnecessary-service-injection-argument](./docs/rules/no-unnecessary-service-injection-argument.md) | Disallow unnecessary argument when injecting service | | | [route-path-style](./docs/rules/route-path-style.md) | Enforces usage of kebab-case (instead of snake_case or camelCase) in route paths | | :wrench: | [use-ember-get-and-set](./docs/rules/use-ember-get-and-set.md) | Enforces usage of Ember.get and Ember.set | +| :white_check_mark: | [no-async-actions](./docs/rules/no-async-actions.md) | Prevents usage of async actions | ### Possible Errors diff --git a/docs/rules/no-async-actions.md b/docs/rules/no-async-actions.md new file mode 100644 index 0000000000..73dd9efa01 --- /dev/null +++ b/docs/rules/no-async-actions.md @@ -0,0 +1,50 @@ +# Do not use async actions.attrs +## Rule `no-async-actions` + + + +Examples of **incorrect** code for this rule: +```js +actions: { + async handleClick() { + // ... + } +} +``` + +```js +actions: { + handleClick() { + return something.then(() => { /* ... */ }); + } +} +``` + +```js +@action +async handleClick() { + // ... +} +``` + +```js +@action +handleClick() { + return something.then(() => { /* ... */ }); +} +``` + + +Examples of **correct** code for this rule: +```js +actions: { + handleClick() { + return nothingOrSomethingThatIsNotAPromise; + } +} +``` + + +## Further Reading + +See http://ember-concurrency.com/docs/tutorial (scroll down to Version 4) From a8dda025786f0b1b0c328077cb15c77a8fb87437 Mon Sep 17 00:00:00 2001 From: Rajasegar Chandran Date: Wed, 29 May 2019 06:32:04 +0530 Subject: [PATCH 3/7] [BUG FIX] Typo fix in rule doc page --- docs/rules/no-async-actions.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/rules/no-async-actions.md b/docs/rules/no-async-actions.md index 73dd9efa01..184a3d7c7e 100644 --- a/docs/rules/no-async-actions.md +++ b/docs/rules/no-async-actions.md @@ -1,4 +1,4 @@ -# Do not use async actions.attrs +# Do not use async actions ## Rule `no-async-actions` From 020b127d5971c57825fadc5bbe248bad16aa38ff Mon Sep 17 00:00:00 2001 From: Rajasegar Chandran Date: Mon, 3 Jun 2019 14:59:54 +0530 Subject: [PATCH 4/7] [CHORE] Review comments --- docs/rules/no-async-actions.md | 4 +++- lib/rules/no-async-actions.js | 1 - tests/lib/rules/no-async-actions.js | 10 +++++----- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/docs/rules/no-async-actions.md b/docs/rules/no-async-actions.md index 184a3d7c7e..649ab2f418 100644 --- a/docs/rules/no-async-actions.md +++ b/docs/rules/no-async-actions.md @@ -47,4 +47,6 @@ actions: { ## Further Reading -See http://ember-concurrency.com/docs/tutorial (scroll down to Version 4) +- Ember Concurrency http://ember-concurrency.com/docs/tutorial (scroll down to Version 4) +- eslint/no-async-promise-executor https://github.com/eslint/eslint/blob/master/docs/rules/no-async-promise-executor.md + diff --git a/lib/rules/no-async-actions.js b/lib/rules/no-async-actions.js index e1f29c5dad..ac7eb67707 100644 --- a/lib/rules/no-async-actions.js +++ b/lib/rules/no-async-actions.js @@ -15,7 +15,6 @@ module.exports = { docs: { description: 'Disallow usage of async actions in components', category: 'Possible Errors', - recommended: true, url: 'http://ember-concurrency.com/docs/tutorial' }, fixable: null, diff --git a/tests/lib/rules/no-async-actions.js b/tests/lib/rules/no-async-actions.js index 5269bec87e..6191560c49 100644 --- a/tests/lib/rules/no-async-actions.js +++ b/tests/lib/rules/no-async-actions.js @@ -26,11 +26,11 @@ ruleTester.run('no-async-actions', rule, { { code: ` Component.extend({ - actions: { - handleClick() { - // ... - } - } + actions: { + handleClick() { + // ... + } + } });`, } From efc6aaf8fe4e6e8ea3079146a2ff62513367c5be Mon Sep 17 00:00:00 2001 From: Rajasegar Chandran Date: Mon, 3 Jun 2019 15:00:33 +0530 Subject: [PATCH 5/7] [CHORE] Yarn update rule no-async-actions --- README.md | 2 +- lib/recommended-rules.js | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 8fe774a04d..693ccfe8cc 100644 --- a/README.md +++ b/README.md @@ -103,7 +103,6 @@ The `--fix` option on the command line automatically fixes problems reported by | :wrench: | [no-unnecessary-service-injection-argument](./docs/rules/no-unnecessary-service-injection-argument.md) | Disallow unnecessary argument when injecting service | | | [route-path-style](./docs/rules/route-path-style.md) | Enforces usage of kebab-case (instead of snake_case or camelCase) in route paths | | :wrench: | [use-ember-get-and-set](./docs/rules/use-ember-get-and-set.md) | Enforces usage of Ember.get and Ember.set | -| :white_check_mark: | [no-async-actions](./docs/rules/no-async-actions.md) | Prevents usage of async actions | ### Possible Errors @@ -111,6 +110,7 @@ The `--fix` option on the command line automatically fixes problems reported by | | Rule ID | Description | |:---|:--------|:------------| | :white_check_mark: | [jquery-ember-run](./docs/rules/jquery-ember-run.md) | Prevents usage of jQuery without Ember Run Loop | +| | [no-async-actions](./docs/rules/no-async-actions.md) | Disallow usage of async actions in components | | :white_check_mark: | [no-attrs-in-components](./docs/rules/no-attrs-in-components.md) | Disallow usage of this.attrs in components | | :white_check_mark: | [no-attrs-snapshot](./docs/rules/no-attrs-snapshot.md) | Disallow use of attrs snapshot in `didReceiveAttrs` and `didUpdateAttrs` | | :white_check_mark: | [no-capital-letters-in-routes](./docs/rules/no-capital-letters-in-routes.md) | Raise an error when there is a route with uppercased letters in router.js | diff --git a/lib/recommended-rules.js b/lib/recommended-rules.js index 06bb6a365c..86f72219cf 100644 --- a/lib/recommended-rules.js +++ b/lib/recommended-rules.js @@ -15,6 +15,7 @@ module.exports = { "ember/local-modules": "off", "ember/named-functions-in-promises": "off", "ember/new-module-imports": "error", + "ember/no-async-actions": "off", "ember/no-attrs-in-components": "error", "ember/no-attrs-snapshot": "error", "ember/no-capital-letters-in-routes": "error", @@ -50,4 +51,4 @@ module.exports = { "ember/routes-segments-snake-case": "error", "ember/use-brace-expansion": "error", "ember/use-ember-get-and-set": "off" -} +} \ No newline at end of file From 85e5e05a5a7b195fa0b57f874468a0c923d38a53 Mon Sep 17 00:00:00 2001 From: Rajasegar Chandran Date: Thu, 20 Jun 2019 12:39:53 +0530 Subject: [PATCH 6/7] [CHORE] Address review comments 1. Make error message consistent across rules and tests by reusing the same. 2. Added brief docs to the rules --- docs/rules/no-async-actions.md | 3 ++- lib/rules/no-async-actions.js | 9 +++++---- tests/lib/rules/no-async-actions.js | 11 +++++------ 3 files changed, 12 insertions(+), 11 deletions(-) diff --git a/docs/rules/no-async-actions.md b/docs/rules/no-async-actions.md index 649ab2f418..ff1e940001 100644 --- a/docs/rules/no-async-actions.md +++ b/docs/rules/no-async-actions.md @@ -1,6 +1,8 @@ # Do not use async actions ## Rule `no-async-actions` +The problem is that it's possible for promise callback to run after the component has been destroyed, +and Ember will complain if you try and call `set()` on a destroyed object. Examples of **incorrect** code for this rule: @@ -48,5 +50,4 @@ actions: { ## Further Reading - Ember Concurrency http://ember-concurrency.com/docs/tutorial (scroll down to Version 4) -- eslint/no-async-promise-executor https://github.com/eslint/eslint/blob/master/docs/rules/no-async-promise-executor.md diff --git a/lib/rules/no-async-actions.js b/lib/rules/no-async-actions.js index ac7eb67707..57f770de10 100644 --- a/lib/rules/no-async-actions.js +++ b/lib/rules/no-async-actions.js @@ -8,7 +8,7 @@ const utils = require('../utils/utils'); //------------------------------------------------------------------------------ -const message = 'Do not use async actions'; +const ERROR_MESSAGE = 'Do not use async actions.'; module.exports = { meta: { @@ -18,6 +18,7 @@ module.exports = { url: 'http://ember-concurrency.com/docs/tutorial' }, fixable: null, + ERROR_MESSAGE, }, create(context) { @@ -31,7 +32,7 @@ module.exports = { if (p.value.async) { context.report({ node: p, - message, + message: ERROR_MESSAGE, }); } else if (body.length === 1 && utils.isReturnStatement(body[0])) { const retSt = body[0]; @@ -39,7 +40,7 @@ module.exports = { retSt.argument.callee.property.name === 'then') { context.report({ node: retSt, - message, + message: ERROR_MESSAGE, }); } } @@ -49,7 +50,7 @@ module.exports = { if (node.value.async) { context.report({ node, - message: 'Do not use async actions' + message: ERROR_MESSAGE }); } } diff --git a/tests/lib/rules/no-async-actions.js b/tests/lib/rules/no-async-actions.js index 6191560c49..7a44daf5c4 100644 --- a/tests/lib/rules/no-async-actions.js +++ b/tests/lib/rules/no-async-actions.js @@ -7,12 +7,11 @@ const rule = require('../../../lib/rules/no-async-actions.js'); const RuleTester = require('eslint').RuleTester; - +const { ERROR_MESSAGE } = rule; //------------------------------------------------------------------------------ // Tests //------------------------------------------------------------------------------ -const message = 'Do not use async actions'; const ruleTester = new RuleTester({ parserOptions: { @@ -47,7 +46,7 @@ ruleTester.run('no-async-actions', rule, { });`, output: null, errors: [{ - message, + message: ERROR_MESSAGE, }] }, { @@ -62,7 +61,7 @@ ruleTester.run('no-async-actions', rule, { });`, output: null, errors: [{ - message, + message: ERROR_MESSAGE, }] }, { @@ -74,7 +73,7 @@ ruleTester.run('no-async-actions', rule, { });`, output: null, errors: [{ - message, + message: ERROR_MESSAGE, }] }, { @@ -88,7 +87,7 @@ ruleTester.run('no-async-actions', rule, { });`, output: null, errors: [{ - message, + message: ERROR_MESSAGE, }] }, From e849ad42e06d6ce53d4f77141a5ed9ae0dc06996 Mon Sep 17 00:00:00 2001 From: Rajasegar Chandran Date: Fri, 21 Jun 2019 13:04:42 +0530 Subject: [PATCH 7/7] [CHORE] Updating docs and adding babel-eslint parser 1. Update the rule docs with new brief 2. Add parser option to invalid tests as babel-eslint --- docs/rules/no-async-actions.md | 4 ++-- tests/lib/rules/no-async-actions.js | 9 +++------ 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/docs/rules/no-async-actions.md b/docs/rules/no-async-actions.md index ff1e940001..db676b21c9 100644 --- a/docs/rules/no-async-actions.md +++ b/docs/rules/no-async-actions.md @@ -1,8 +1,8 @@ # Do not use async actions ## Rule `no-async-actions` -The problem is that it's possible for promise callback to run after the component has been destroyed, -and Ember will complain if you try and call `set()` on a destroyed object. +Using async actions can lead to memory leaks and application errors if you +don't check for `isDestroying` and `isDestroyed` after each async step Examples of **incorrect** code for this rule: diff --git a/tests/lib/rules/no-async-actions.js b/tests/lib/rules/no-async-actions.js index 7a44daf5c4..8d5372e99d 100644 --- a/tests/lib/rules/no-async-actions.js +++ b/tests/lib/rules/no-async-actions.js @@ -13,12 +13,7 @@ const { ERROR_MESSAGE } = rule; //------------------------------------------------------------------------------ -const ruleTester = new RuleTester({ - parserOptions: { - ecmaVersion: 2018, - sourceType: 'module', - } -}); +const ruleTester = new RuleTester({}); ruleTester.run('no-async-actions', rule, { valid: [ @@ -71,6 +66,7 @@ ruleTester.run('no-async-actions', rule, { // ... } });`, + parser: 'babel-eslint', output: null, errors: [{ message: ERROR_MESSAGE, @@ -85,6 +81,7 @@ ruleTester.run('no-async-actions', rule, { }); } });`, + parser: 'babel-eslint', output: null, errors: [{ message: ERROR_MESSAGE,