[NEW RULE] Adding no-async-actions rule [DO NOT MERGE] #424
Conversation
Fixes ember-cli#418 [DO NOT MERGE] 1. Working for normal async actions 2. With decorators it is throwing errors, need to fix this.
1. Also adding the rule to the Best practices section in README
tests/lib/rules/no-async-actions.js
Outdated
| { | ||
| code: ` | ||
| Component.extend({ | ||
| actions: { |
There was a problem hiding this comment.
This is unreadable without proper indentation.
There was a problem hiding this comment.
Indented propertly
README.md
Outdated
| @@ -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 | | |||
There was a problem hiding this comment.
Remember to run yarn update and add the rule to index.js too.
| # Do not use async actions | ||
| ## Rule `no-async-actions` | ||
|
|
||
|
|
There was a problem hiding this comment.
I'm a bit unclear on why we should not use async actions. Can you explain that more and include in the doc here?
There was a problem hiding this comment.
Me too not very clear about the requirement, i am exploring , will add more info once ready
There was a problem hiding this comment.
Let's add at least a brief explanation of the reasoning in this doc.
lib/rules/no-async-actions.js
Outdated
| docs: { | ||
| description: 'Disallow usage of async actions in components', | ||
| category: 'Possible Errors', | ||
| recommended: true, |
There was a problem hiding this comment.
Rules should never be added as recommended since that would be a breaking change. We can consider making it recommended in the future.
There was a problem hiding this comment.
Removed the recommended flag
|
@bmish Thanks for reviewing, addressed the initial review comments |
| }); | ||
|
|
||
| ruleTester.run('no-async-actions', rule, { | ||
| valid: [ |
There was a problem hiding this comment.
We need a valid example to ensure this rule does not trigger on actions in an irrelevant object/class. It should only trigger on the actions key inside an Ember controller/component/route.
There was a problem hiding this comment.
^ still waiting on this rule to be limited to actions inside components only.
tests/lib/rules/no-async-actions.js
Outdated
| // Tests | ||
| //------------------------------------------------------------------------------ | ||
|
|
||
| const message = 'Do not use async actions'; |
There was a problem hiding this comment.
message should be imported from the rule file. See the no-test-and-then rule for an example.
lib/rules/no-async-actions.js
Outdated
| if (node.value.async) { | ||
| context.report({ | ||
| node, | ||
| message: 'Do not use async actions' |
There was a problem hiding this comment.
This should use the message variable defined at the top of the file.
| # Do not use async actions | ||
| ## Rule `no-async-actions` | ||
|
|
||
|
|
There was a problem hiding this comment.
Let's add at least a brief explanation of the reasoning in this doc.
1. Make error message consistent across rules and tests by reusing the same. 2. Added brief docs to the rules
| docs: { | ||
| description: 'Disallow usage of async actions in components', | ||
| category: 'Possible Errors', | ||
| url: 'http://ember-concurrency.com/docs/tutorial' |
There was a problem hiding this comment.
Please rebase on the latest master to pickup the prettier changes.
| }); | ||
|
|
||
| ruleTester.run('no-async-actions', rule, { | ||
| valid: [ |
There was a problem hiding this comment.
^ still waiting on this rule to be limited to actions inside components only.
1. Update the rule docs with new brief 2. Add parser option to invalid tests as babel-eslint
|
|
||
| ```js | ||
| @action | ||
| async handleClick() { |
There was a problem hiding this comment.
why forbid this? This is totally valid
| @@ -0,0 +1,53 @@ | |||
| # Do not use async actions | |||
| ## Rule `no-async-actions` | |||
There was a problem hiding this comment.
Instead, could we throw a lint error only after an await AND access of this?
| @@ -0,0 +1,53 @@ | |||
| # Do not use async actions | |||
| ## Rule `no-async-actions` | |||
There was a problem hiding this comment.
Instead, could we throw a lint error only after an await AND access of this?
Fixes #418