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

Docs: improve documentation of no-return-await #13215

Merged
merged 4 commits into from May 14, 2020

Conversation

LinusU
Copy link
Contributor

@LinusU LinusU commented Apr 23, 2020

this is a resubmit of eslint/archive-website#716 but to the correct repo ☺️

Prerequisites checklist

  • I have read the contributing guidelines.
  • The team has reached consensus on the changes proposed in this pull request. If not, I understand that the evaluation process will begin with this pull request and won't be merged until the team has reached consensus.

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

[x] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

First of all, I know that there have been a lot of discussions on wether this rule is a good one or not. I do not want to bring up that discussion here at all. I just want to clarify the documentation to better explain the implications, I don't want to advocate for either turning this rule on or off.


I've had some team member asking why we are using return await, especially after reading the current version of the documentation for no-return-await. The reason we are using return await is because otherwise the functions are missing from the stack traces if an error is thrown asynchronously in the promise that is being returned.

My intention with this change is to document that pitfall, and to clarify exactly what is being skipped when not using return await (that is, that the function is either still on the call stack, or not)

I'm very open to suggestion on more improvements that we can do here! ❤️

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

n/a

@eslint-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Apr 23, 2020
docs/rules/no-return-await.md Outdated Show resolved Hide resolved
docs/rules/no-return-await.md Outdated Show resolved Hide resolved
@kaicataldo kaicataldo added documentation Relates to ESLint's documentation evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Apr 24, 2020
LinusU and others added 2 commits April 25, 2020 10:57
Co-Authored-By: Kai Cataldo <kai@kaicataldo.com>
Co-Authored-By: Kai Cataldo <kai@kaicataldo.com>
@LinusU
Copy link
Contributor Author

LinusU commented Apr 25, 2020

Thanks @kaicataldo! I incorporated both of your changes.

Let me know if you want me to squash this ☺️

@LinusU
Copy link
Contributor Author

LinusU commented May 7, 2020

@kaicataldo anything else I can do to move this forward? ☺️

Copy link
Member

@kaicataldo kaicataldo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay! I have a few suggestions around organization/wording, but this generally LGTM and I think outlines the trade offs well. Thanks for working on this!

docs/rules/no-return-await.md Outdated Show resolved Hide resolved
docs/rules/no-return-await.md Outdated Show resolved Hide resolved
docs/rules/no-return-await.md Outdated Show resolved Hide resolved
@kaicataldo kaicataldo removed the evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion label May 14, 2020
Co-authored-by: Kai Cataldo <kai@kaicataldo.com>
@LinusU
Copy link
Contributor Author

LinusU commented May 14, 2020

@kaicataldo No worries, great suggestions!

I've committed them ✔️

Copy link
Member

@kaicataldo kaicataldo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you!

@kaicataldo kaicataldo merged commit 82a448a into eslint:master May 14, 2020
@LinusU LinusU deleted the doc-no-return-await branch May 14, 2020 20:26
@LinusU
Copy link
Contributor Author

LinusU commented May 14, 2020

Awesome, thanks for the merge! 👏

@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Nov 11, 2020
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Nov 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion documentation Relates to ESLint's documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants