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

feat(rule) Warn for awaits without a try/catch wrapper #201

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

can-keklik
Copy link

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • New rule
  • Changes an existing rule
  • Add autofixing to a rule
  • Other, please explain:

What changes did you make? (Give an overview)

I've created a new rule named wrap-await-with-try-catch, which warns you if you don’t have try/catch around your awaits. For instance,

This would cause a warning:

await doSomething()

And this one is corrected:

try {
    await doSomething();
}
catch (ex) {
    //...
}

The rule basically traverses ancestors of the await expression to find a try block that has a catch clause. It was slightly trickier to implement the idea, though. If we simply looked for a TryStatement parent, the await, which is wrapped by a catch or a finally block, could be recognized as valid since catch and finally are children of try. For instance,

In the following await still has a TryStatement as an ancestor, but it is invalid for our case:

try {
    ...
}
catch (ex) {
    await doSomething();
}

And one of the possible fixes is below. The inner try/catch is ignored, we accept the outer one:

try {
    try {
        ...
    }
    catch (ex) {
        await doSomething();
    }
}
catch (ex) {
}

That is, the await expression must be in a try block, no matter how much nested it is. I don't offer a fixer, since the possible scenarios can be too complex to fix automatically.

@xjamundx
Copy link
Contributor

xjamundx commented Feb 11, 2021

Thanks for submitting this. I think I'm pretty happy merging this in.

Any thoughts on things like?

await doSomething().catch(handleError)

Which, honestly is my preference a lot of the time.

Should that also throw an error? IF this is more about style then definitely yes. If it's about correctness and safety I think it's a maybe.

@can-keklik
Copy link
Author

can-keklik commented Feb 13, 2021

Hello,

Thanks :)

Well, to be honest, I considered this as more of a warning about style. Using try/catch prevents most of the confusions about handling Promise errors imo. I admit that it introduces some verbosity, but still useful in terms of forcing a single style.

So yes, I think it should throw an error for that case too. Nonetheless, I am open to suggestions. We can think about adding an exception for .catch(handler) if that is common or requested by majority.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants