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

Rule against await await #1168

Open
not-my-profile opened this issue Jun 16, 2023 · 15 comments
Open

Rule against await await #1168

not-my-profile opened this issue Jun 16, 2023 · 15 comments

Comments

@not-my-profile
Copy link
Contributor

While the number of await keywords can affect which promise job is executed first (because promise jobs have to be enqued even if the promise is fulfilled), in sane code you usually only ever want one await keyword for one expression. And multiple await keywords are usually just an accident, so it would be nice to have a rule to detect this.

E.g. Invalid:

await await fetch('/test');

Valid:

await fetch('/test');
@bartlomieju
Copy link
Member

Sounds reasonable, PRs are welcome

@dsherret
Copy link
Member

dsherret commented Jul 14, 2023

Has anyone ever written this kind of code though? Is a rule worth it considering the extra time necessary to check for this condition? Perhaps there is precedence in other linters?

@dsherret
Copy link
Member

It seems people write this kind of code: https://github.com/search?type=code&q=%22await+await+%22+language%3AJavaScript&l=JavaScript (maybe when coming from other languages like c# where this is necessary)

@not-my-profile
Copy link
Contributor Author

I have written this code by accident ... you can easily end up with await await when copy'n'pasting code.

@dsherret
Copy link
Member

Maybe deno fmt should just remove multiple awaits.

@not-my-profile
Copy link
Contributor Author

As I explained in the issue description await await can result in different behavior than just await, so I think this change would be too dangerous for deno fmt, which I'd expect to never change the semantics of code.

@nayeemrmn
Copy link
Contributor

I'm surprised the tsc warning isn't shown in this case

'await' has no effect on the type of this expression. deno-ts(80007)

While the number of await keywords can affect which promise job is executed first

Does that really happen when awaiting a non-promise?

@not-my-profile
Copy link
Contributor Author

not-my-profile commented Jul 14, 2023

I'm surprised the tsc warning isn't shown in this case

Oh, ... yeah me too. In case we get that warning to work then this lint is probably redundant.

Does that really happen when awaiting a non-promise?

Yes, according to the ECMAScript standard it even must happen.

@dsherret
Copy link
Member

It does affect it, but I wouldn't consider it too dangerous. That said, considering it does change execution that's reasonable to not do it for formatting.

I'm surprised the tsc warning isn't shown in this case

image

Considering this is already handled by tsc, I think we should not add this to deno_lint. I'm going to close this issue.

@dsherret dsherret closed this as not planned Won't fix, can't repro, duplicate, stale Jul 14, 2023
@not-my-profile
Copy link
Contributor Author

I guess we should open a new issue at denoland/deno about that tsc warning not being reported for await await fetch('/test');?

@dsherret
Copy link
Member

dsherret commented Jul 14, 2023

I guess we should open a new issue at denoland/deno about that tsc warning not being reported for await await fetch('/test');?

It is reported as a warning in the editor.

image

@not-my-profile
Copy link
Contributor Author

But not by deno check?

@nayeemrmn
Copy link
Contributor

image

Ah, seems to work inconsistently:
image

@dsherret
Copy link
Member

dsherret commented Jul 14, 2023

Oh yeah. Looks like typescript is only reporting this as a suggestion diagnostic, which we don't surface except in the editor. Perhaps it being reported via deno lint would still be good. I'm going to reopen.

@dsherret dsherret reopened this Jul 14, 2023
@not-my-profile
Copy link
Contributor Author

Slightly getting off-topic but would it make sense to add a deno check flag to also report suggestion diagnostics? Although it seems like the tsc CLI doesn't have such a flag.

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

No branches or pull requests

4 participants