-
-
Notifications
You must be signed in to change notification settings - Fork 202
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: no-async-actions #418
Comments
Fixes ember-cli#418 [DO NOT MERGE] 1. Working for normal async actions 2. With decorators it is throwing errors, need to fix this.
@Turbo87 For the decorators, should we need to configure some parser options i am getting the following error. Any help would be appreciated. |
good question, I don't know 😅 we might have to configure the babel-eslint parser for those cases |
@Turbo87 I need your help in formulating the docs for this rule @bmish needs some clarifications here |
no, it's unrelated
⬆️ |
@Turbo87 Then, can you please help me in coming up with a proper brief about the rule |
@bmish Regarding the error above, any help would be appreciated. |
as written in the initial post:
tl;dr: using async actions can lead to memory leaks and application errors if you don't check for |
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. |
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 Given the stated goal is to avoid errors from not checking |
@mongoose700 this rule will be optional, if you don't want it, then you don't have to enable it.
you can trigger it, there is nothing wrong with that. it's just that doing anything with |
Even so, I think it should do a better job at targeting the behavior it's intended to discourage.
Then the rule should be focused on anything that may modify |
yes, unless you know about or alternatively one can use
you are very welcome to work on improving this rule 😉 |
Is there a reason this is targeting actions specifically, as opposed to any member functions of |
@bendemboski there is a reason why targeting actions is a good idea. The main reason why you would want to add If you use ember-concurrency this is handled for you automatically so I think it's reasonable to just flat out warn against using |
@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:
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:
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 A couple of ways I could see improving the situation somewhat:
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 |
see http://ember-concurrency.com/docs/tutorial (scroll down to Version 4)
Bad:
Good:
The text was updated successfully, but these errors were encountered: