Skip to content
Open
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -110,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 |
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


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.

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 @@ -51,4 +52,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
});
}
}
}
}
};
}
};
95 changes: 95 additions & 0 deletions tests/lib/rules/no-async-actions.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
'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({
parserOptions: {
ecmaVersion: 2018,
sourceType: 'module',
}
});

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() {
// ...
}
});`,
output: null,
errors: [{
message: ERROR_MESSAGE,
}]
},
{
code: `Component.extend({
@action
handleClick() {
return something.then(() => {
let hello = "world";
});
}
});`,
output: null,
errors: [{
message: ERROR_MESSAGE,
}]
},

]
});