Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[NEW RULE] Adding no-async-actions rule [DO NOT MERGE] #424

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,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 |
Expand Down
53 changes: 53 additions & 0 deletions docs/rules/no-async-actions.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
# Do not use async actions
## Rule `no-async-actions`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead, could we throw a lint error only after an await AND access of this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead, could we throw a lint error only after an await AND access of this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

example?: #1421


Using async actions can lead to memory leaks and application errors if you
don't check for `isDestroying` and `isDestroyed` after each async step

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit unclear on why we should not use async actions. Can you explain that more and include in the doc here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Me too not very clear about the requirement, i am exploring , will add more info once ready

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add at least a brief explanation of the reasoning in this doc.


Examples of **incorrect** code for this rule:
```js
actions: {
async handleClick() {
// ...
}
}
```

```js
actions: {
handleClick() {
return something.then(() => { /* ... */ });
}
}
```

```js
@action
async handleClick() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why forbid this? This is totally valid

// ...
}
```

```js
@action
handleClick() {
return something.then(() => { /* ... */ });
}
```


Examples of **correct** code for this rule:
```js
actions: {
handleClick() {
return nothingOrSomethingThatIsNotAPromise;
}
}
```


## Further Reading

- Ember Concurrency http://ember-concurrency.com/docs/tutorial (scroll down to Version 4)

3 changes: 2 additions & 1 deletion lib/recommended-rules.js
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -52,4 +53,4 @@ module.exports = {
"ember/routes-segments-snake-case": "error",
"ember/use-brace-expansion": "error",
"ember/use-ember-get-and-set": "off"
}
}
61 changes: 61 additions & 0 deletions lib/rules/no-async-actions.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
'use strict';

const utils = require('../utils/utils');


//------------------------------------------------------------------------------
// General rule - Don't use async actions
//------------------------------------------------------------------------------


const ERROR_MESSAGE = 'Do not use async actions.';

module.exports = {
meta: {
docs: {
description: 'Disallow usage of async actions in components',
category: 'Possible Errors',
url: 'http://ember-concurrency.com/docs/tutorial'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rebase on the latest master to pickup the prettier changes.

},
fixable: null,
ERROR_MESSAGE,
},

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: ERROR_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: ERROR_MESSAGE,
});
}
}
});
} else if (node.decorators) {
if (node.decorators.find(d => d.expression.name === 'action')) {
if (node.value.async) {
context.report({
node,
message: ERROR_MESSAGE
});
}
}
}
}
};
}
};
92 changes: 92 additions & 0 deletions tests/lib/rules/no-async-actions.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
'use strict';

//------------------------------------------------------------------------------
// Requirements
//------------------------------------------------------------------------------

const rule = require('../../../lib/rules/no-async-actions.js');
const RuleTester = require('eslint').RuleTester;

const { ERROR_MESSAGE } = rule;
//------------------------------------------------------------------------------
// Tests
//------------------------------------------------------------------------------


const ruleTester = new RuleTester({});

ruleTester.run('no-async-actions', rule, {
valid: [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@bmish bmish Jun 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^ still waiting on this rule to be limited to actions inside components only.

{
code: `
Component.extend({
actions: {
handleClick() {
// ...
}
}
});`,
}

],

invalid: [
{
code: `Component.extend({
actions: {
async handleClick() {
// ...
}
}
});`,
output: null,
errors: [{
message: ERROR_MESSAGE,
}]
},
{
code: `Component.extend({
actions: {
handleClick() {
return something.then(() => {
let hello = "world";
});
}
}
});`,
output: null,
errors: [{
message: ERROR_MESSAGE,
}]
},
{
code: `Component.extend({
@action
async handleClick() {
// ...
}
});`,
parser: 'babel-eslint',
output: null,
errors: [{
message: ERROR_MESSAGE,
}]
},
{
code: `Component.extend({
@action
handleClick() {
return something.then(() => {
let hello = "world";
});
}
});`,
parser: 'babel-eslint',
output: null,
errors: [{
message: ERROR_MESSAGE,
}]
},

]
});