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: prefer-async-await rule (fixes #9649) #11962

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
2 participants
@mdjermanovic
Copy link
Contributor

commented Jul 7, 2019

What is the purpose of this pull request? (put an "X" next to item)

[X] New rule #9649

What changes did you make? (Give an overview)

  • Checks only method calls, like foo.then()
  • Reports .then(), .catch() and .finally()
  • Has two four options, to allow at top level, in getters/setters, await and yield expr.
  • The main example is designed to show advantages of async/await

Is there anything you'd like reviewers to focus on?

@eslint eslint bot added the triage label Jul 7, 2019

@ljharb
Copy link
Contributor

left a comment

What about an often-intentional line like:

await foo.catch(…)

?

I’d hope that wouldn’t warn here, since by using await already I’ve made it clear that this is as far as i want to make it imperative.

@mdjermanovic

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2019

What about an often-intentional line like:

await foo.catch(…)

?

I’d hope that wouldn’t warn here, since by using await already I’ve made it clear that this is as far as i want to make it imperative.

I'll add an option to allow calls in await expressions, and another option for yield expressions.

@ljharb

This comment has been minimized.

Copy link
Contributor

commented Jul 7, 2019

Also, changing then calls to await is only equivalent when it’s in a chain - iow, we should be careful this rule doesn’t encourage incorrect and unnecessary serialization.

@mdjermanovic

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2019

Also, changing then calls to await is only equivalent when it’s in a chain - iow, we should be careful this rule doesn’t encourage incorrect and unnecessary serialization.

Yes, 'Promise methods' might not be clear enough. I'll try to clarify that in the doc, and add a 'correct' example with Promise.all()

@mdjermanovic

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2019

And maybe the doc should use the term 'Promise.prototype methods' or 'Promise prototype methods' instead of just 'Promise methods'.

@mdjermanovic

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2019

Added await and yield options. Also, 'Promise methods' are now 'Promise prototype methods', there is a Promise.all() section and a couple more sentences to clarify.

@mdjermanovic

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2019

Also, changing then calls to await is only equivalent when it’s in a chain - iow, we should be careful this rule doesn’t encourage incorrect and unnecessary serialization.

Did you mean something like this:

function foo(x, y) {
  f(x).then(a => {...});
  g(y).then(b => {...});
}

the rule reports warnings and someone changes it to:

async function foo(x, y) {
  const a = await f(x);
  ...
  const b = await g(y);
  ...
}

which is an unnecessary serialization

@ljharb

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2019

Yes, exactly. That makes code worse, not better.

@mdjermanovic

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2019

What could the rule do to prevent that?

Maybe not report unless an additional rule option is explicitly set. If it is set, then report but show a different message suggesting to split the body into several async functions (there would be an example in the docs).

@ljharb

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2019

This is the difficulty with lint rules pushing await at all, and why imo require-await is such a pervasively terrible rule.

await is sugar for nested promise thens or a chain; it should not be used otherwise.

The difficulty, then, is that await can’t be safely recommended unless you are absolutely certain that there are no other un-awaited async operations that would be initiated after the point you want to insert the await. To do this safely requires a knowledge of every function’s runtime behavior (ie, if there’s any function calls or accessors, you have to know if they return promises or not).

@mdjermanovic

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2019

The difficulty, then, is that await can’t be safely recommended unless you are absolutely certain that there are no other un-awaited async operations that would be initiated after the point you want to insert the await. To do this safely requires a knowledge of every function’s runtime behavior (ie, if there’s any function calls or accessors, you have to know if they return promises or not).

Since that's beyond the static analysis, maybe the rule could check only 'last' statements (after which the function will end), or perhaps just return statements. In fact, the async version can be truly equivalent only if the then() was returned in the non-async version?

Though, I'm not sure would such a rule be useful, or just confusing e.g. in cases where there are unrelated then()s in the same function.

@mdjermanovic

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2019

And for those that are not returned, the message could tell to extract the code into a new async function.

@mdjermanovic

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2019

For example, something like this (with better messages):

function foo(x, y) {
  f(x).then(a => {...}); // Error: Extract to an async function
  return g(y).then(b => {...}); // Error: Use async/await syntax
}
@ljharb

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2019

The issue there is indeed that if you add await in front of f(x), then you're artificially delaying when g can be invoked; so the only safe way to write it with await is either:

function foo(x, y) {
  f(x).then(a => {...}); // Error: Extract to an async function
  const b = await g(y);
  return ...;
}

or, if a human but not a linter wrote it (obv depending on what's in the ellipses):

function foo(x, y) {
  const [a, b] = await Promise.all([f(x), g(y)]);
  processA(a);
  return processB(b);
}

I'm not sure how valuable a linter rule is that can only safely recommend the former change, nor do i think it's objectively cleaner.

@mdjermanovic

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2019

The Promise.all() version is not equivalent, it waits for both before processing both?

The idea was to change this:

function foo(x, y) {
  f(x).then(a => {...}); // Error: Extract to an async function
  return g(y).then(b => {...}); // Error: Use async/await syntax
}

into this:

async function foo(x, y) {
  newFunction(x);
  const b = await g(y);
  ...
}

async newFunction(x) {
  const a = await f(x);
  ...
}

and there should be perhaps a try-catch somewhere to be fully equivalent.

@ljharb

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2019

ah, gotcha. that would work but is a bit of a major change for an eslint rule to recommend, imo.

@mdjermanovic

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2019

ah, gotcha. that would work but is a bit of a major change for an eslint rule to recommend, imo.

I guess it depends on the user.

If someone wants to completely switch to the async/await, this might be seen as a necessary step - is there another way to convert this code into an async/await equivalent? There can be an example in the docs.

If someone does not want major changes like this, there could be an option to check only return statements, or not use the rule at all.

Just to note, a new function declaration from the example is what a human can do (it should have a meaningful name etc.), autofix would probably be an anon async iife in the same place. It doesn't look much better on a small example, but if it is a replacement for a chain/nested thens, it should be more readable. Though, I'm not sure is this rule supposed to have autofix, it would be more like a transpiler than a fixer.

@ljharb

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2019

You can't completely "switch" because async/await a) is not complete (there's no Promise.all) and b) you simply can't "not know about Promises" - they will always be present, even in a codebase using async/await, if the code is optimal.

@mdjermanovic

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2019

You can't completely "switch" because async/await a) is not complete (there's no Promise.all) and b) you simply can't "not know about Promises" - they will always be present, even in a codebase using async/await, if the code is optimal.

Of course, the whole point of this rule is just to restrict .then(), .catch(), .finally() in favor of async, await, try {} catch {} finally{}, as described in #9649 with the reasons why is it useful.

And, as you said, it's important that this rule doesn’t encourage incorrect and unnecessary serialization, i.e. make the code worse. If it is impossible to achieve, then the rule probably shouldn't be included in the core rules, although the request is accepted.

I believe that this version should be modified to report different messages when a restricted call is in a return statement and when it isn't, all that with appropriate examples of how to change the code in the rule documentation.

The question is - is it good enough? Can it still make someone's code worse? Is it useful at all?

@ljharb

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2019

My intuition is that any rule that attempts to encourage usage of await is not, on balance, useful.

@mdjermanovic

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2019

Then I'll modify it and move to a plugin, perhaps it might be useful for someone thanks to this analysis.

Btw. VSCode can auto-convert chains (and nested calls), and it works by design only with returned ones.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.