New Rule: no-async-actions

[Turbo87]

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

Bad:

actions: {
  async handleClick() {
    // ...
  }
}

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

@action
async handleClick() {
  // ...
}

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

Good:

actions: {
  handleClick() {
    return nothingOrSomethingThatIsNotAPromise;
  }
}

[rajasegar]

@Turbo87 For the decorators, should we need to configure some parser options i am getting the following error. Any help would be appreciated.

[Turbo87]

good question, I don't know 😅

we might have to configure the babel-eslint parser for those cases

[rajasegar]

@Turbo87 I need your help in formulating the docs for this rule Is it on the same lines with eslint/no-async-promise-executor rule in eslint https://github.com/eslint/eslint/blob/master/docs/rules/no-async-promise-executor.md

@bmish needs some clarifications here https://github.com/ember-cli/eslint-plugin-ember/pull/424#discussion_r289675593

[Turbo87]

Is it on the same lines with eslint/no-async-promise-executor rule in eslint

no, it's unrelated

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

⬆️

[rajasegar]

@Turbo87 Then, can you please help me in coming up with a proper brief about the rule

[rajasegar]

@bmish Regarding the error above, any help would be appreciated.

[Turbo87]

as written in the initial post:

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

tl;dr: using async actions can lead to memory leaks and application errors if you don't check for isDestroying and isDestroyed after each async step

[rajasegar]

Sounds good to a brief, but do we also need to mention how to fix this error or any recommended practice to avoid async actions.

[mongoose700]

I don't think this rule makes sense. There are going to be cases where we want to trigger asynchronous behavior through actions, but this doesn't address how we would do so. The good example is returning something other than a promise, but if we just had a promise that didn't get returned, this lint rule would just ignore it.

Given the stated goal is to avoid errors from not checking isDestroying or isDestroyed, it would make more sense for the lint rule to focus on that. If someone did correctly check those, it would still fail the lint rule as it's currently written. And if someone does handle it correctly, they shouldn't have to then manually disable the rule.

[Turbo87]

@mongoose700 this rule will be optional, if you don't want it, then you don't have to enable it.

There are going to be cases where we want to trigger asynchronous behavior through actions

you can trigger it, there is nothing wrong with that. it's just that doing anything with this after the async behavior has finished can be very dangerous.

[mongoose700]

@mongoose700 this rule will be optional, if you don't want it, then you don't have to enable it.

Even so, I think it should do a better job at targeting the behavior it's intended to discourage.

you can trigger it, there is nothing wrong with that. it's just that doing anything with this after the async behavior has finished can be very dangerous.

Then the rule should be focused on anything that may modify this asynchronously, not on just whether the function is async or returns a promise. If someone has an action that violates this rule, they are more likely to fix it by making it stop returning the promise than by adding the proper checks, especially since the error message is simply Do not use async actions. Do you believe that async actions should be avoided entirely? The rule would make the entire example in the tutorial invalid.

[Turbo87]

Do you believe that async actions should be avoided entirely?

yes, unless you know about isDestroying and isDestroyed and promise to always apply them properly.

or alternatively one can use ember-concurrency which avoids these issues completely...

I think it should do a better job at targeting the behavior it's intended to discourage

you are very welcome to work on improving this rule 😉

[bendemboski]

Is there a reason this is targeting actions specifically, as opposed to any member functions of EmberObject or glimmer Component sub-classes? Seems like the pitfalls aren't unique to "actions," and the @action decorator no longer even really implies that it's called from a template, so targeting "actions" seems a little arbitrary?

[mansona]

@bendemboski there is a reason why targeting actions is a good idea. The main reason why you would want to add @action in the first place is because you want to have the ability to access this. in your function. If you access this. after an await in an async action then there is a possibility that this. doesn't exist any more.

If you use ember-concurrency this is handled for you automatically so I think it's reasonable to just flat out warn against using async with actions. There may be an argument to make this rule super smart and check if you're accessing this. after an await call, but that seems a bit overkill, and if you're wanting async with your actions you will likely have a much better time just using ember-concurrency.

[bendemboski]

@mansona I'm still not sold. Everything you said makes sense, but I think this will only partially address what it's aiming for and could introduce further confusion. Async scenarios that this won't cover:

1. A non-async action that calls an async function (without await'ing obviously)
2. An async function (not action) on a component triggered and this-bound by some eventing mechanism, e.g. @ember/object/events or eventemitter3
3. An async function on a service

All of the above have the same pitfalls as the targeted scenario, and only differ from the targeted scenario in that they are less common in practice (probably, although I think async service functions are pretty common).

Further points of confusion:

1. The rule says that it's targeting components, but looking at the PR, it doesn't look like that's the case? Maybe I'm mis-reading, but it looks like it will flag any async function decoration by @action, whether in a component or not.
2. The original rationale (version 4 of http://ember-concurrency.com/docs/tutorial/) is a kinda deprecated scenario (not technically deprecated I don't think, but clearly not Modern Ember) -- using Ember.set(). There's no runtime assertion if you set a @tracked property after its object is destroyed, and there's no inherent harm -- the autotracking system is unrelated to the isDestroying/isDestroyed, so its just like setting a property on a {} that your application logic has "stopped using".

So it looks like this is partially addressing a problem that is not clearly rationalized for modern Ember code. Given that the problem itself, especially when considered in the absence of Ember.set(), is not at all specific to Ember (you always have to be careful with async functions inside class methods!), and @action already kinda confuses people and suggests that it's more "magical" than it is in non-EmberObject usages, I really worry that this rule will end up doing more harm than good.

A couple of ways I could see improving the situation somewhat:

1. Target only EmberObjects, or perhaps only Ember.Components and Ember.Controllers (because they are much more likely to be using Ember.set() rather than @tracked)
2. Do the "deeper" parsing of the function, looking for Ember.set() invocations, so it's really the lint-time warning about that specific runtime assertion.
3. Rationalize it as a use-ember-concurrency-for-async-behavior or something, where any async member function anywhere is flagged (perhaps with some post-await this checks or something). This would be putting a rule that's not at all Ember-specific in Ember's linting package, but I could see justifying that because we have a specific recommendation to make (ember-concurrency).

Regardless, at the very least I think we need to understand whether this rule is actually meant to target only components (and ensure that the implementation agrees with that), and if it's meant to target more scenarios than just the Ember.set() assertion, provide document/examples of some of those other scenarios and the related pitfalls.