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

require-await Potential pitfall if used. Doc update maybe? #9540

Closed
KpjComp opened this issue Oct 30, 2017 · 7 comments

Comments

@KpjComp
Copy link

commented Oct 30, 2017

Been using ESLint for a while, most rules are great.
But I would like to point out there is a major problem in enabling this rule.

The problem, take the code below.

async function test() { throw new Error("test"); }

There is no await, but what the async does here is guarantee a Promise. So the following code will return got.

test().catch(function (e) { console.log("got"); })

Take away the async and the above code is going to fail big time Now if you can guarantee you are always going to call test with await, you are ok again as await will check if it's a Promise, but if you sent this to any other other Promise based consumers you have the above issue.

I'm mainly pointing this out as the Doc's say nothing of this issue, and could potential cause hard to debug issues for people who are currently mixing Promise & async / await. So I wonder if a big warning of this potential pitfall should be put in the Doc's.

@eslint eslint bot added the triage label Oct 30, 2017

@not-an-aardvark

This comment has been minimized.

Copy link
Member

commented Oct 30, 2017

Seems reasonable to me. There was more discussion on potential pitfalls in #6820.

@GCrispino

This comment has been minimized.

Copy link

commented Feb 23, 2018

Is there anything else that could be done about this issue?

The docs have a "When Not To Use It" section, that apparently warns the user that this rule may bring some harm to their code. So it seems to me that this is a matter of using or not using the rule.

Any more thoughts?

@santanaG

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2018

Given @GCrispino 's point. What should be done about this?

@dhruvdutt

This comment has been minimized.

Copy link

commented Aug 21, 2018

I would love to work on this. Can someone please describe the spec and point to some code?

@kaicataldo

This comment has been minimized.

Copy link
Member

commented Oct 15, 2018

@dhruvdutt Great! The documentation file in question is here. What do you mean by "describe the spec"?

@NYCJacob

This comment has been minimized.

Copy link

commented Oct 19, 2018

What if you just added a couple of sentences to the rule details explaining the danger?

@nzakas nzakas self-assigned this Nov 7, 2018

@nzakas

This comment has been minimized.

Copy link
Member

commented Nov 7, 2018

I'll take a stab at this.

nzakas added a commit that referenced this issue Nov 7, 2018

@nzakas nzakas closed this in 57f357e Nov 9, 2018

@eslint eslint bot locked and limited conversation to collaborators May 9, 2019

@eslint eslint bot added the archived due to age label May 9, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
9 participants
You can’t perform that action at this time.