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

[no-misused-promises] [DOCS] examples should include a pattern for fixing async callbacks #3892

Closed
thw0rted opened this issue Sep 16, 2021 · 2 comments · Fixed by #3898
Closed
Labels
documentation Documentation ("docs") that needs adding/updating package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@thw0rted
Copy link
Contributor

(I didn't see a template for Docs issues, and the one I picked for eslint-plugin had no relevant fields -- gonna make one up as I go along, sorry.)

Affected page

https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/docs/rules/no-misused-promises.md

Current examples

Under the checksVoidReturn section, there are "bad" examples like this:

new Promise(async (resolve, reject) => {
  await doSomething();
  resolve();
});

const eventEmitter = new EventEmitter();
eventEmitter.on('some-event', async () => {
  await doSomething();
});

The "fixes" for these examples look like

new Promise((resolve, reject) => {
  // Do something
  resolve();
});

const eventEmitter = new EventEmitter();
eventEmitter.on('some-event', () => {
  doSomething();
});

Basically, the docs say "just stop using an async function". Obviously, if the previous code was declared async, the dev expected to await something and will have to refactor, sometimes very significantly, to use a non-async callback.

Suggested improvement

Give more concrete examples, like Brad did in #1637 (comment) :

emitter.on('some-event', function() {
  async function asyncWorker() {
    let response = await fetch('/some/thing');
  }

  asyncWorker().then(handleSuccess, handleError);
});

That's a useful, concrete pattern to follow for refactoring a previously-async callback to safely avoid returning an orphaned Promise to an event manager that didn't expect to get one (and would ignore its rejection).

@thw0rted thw0rted added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for maintainers to take a look labels Sep 16, 2021
@bradzacher
Copy link
Member

we don't have a template for docs because the vast majority of the time you can PR a change to the rule docs faster and easier than you can explain them!

Basically, the docs say "just stop using an async function".

That's not the intention - but it's easy to interpret them that way.
There's an implicit convention in some ESLint docs wherein the "valid" cases mirror the "invalid" cases and show how to fix them.
Though some docs will just show the two sets with no correlation.

no-misused-promises is the second set - but that's not clear because it uses the same shape of examples for both.


happy to accept a PR to clarify the docs.

@bradzacher bradzacher added documentation Documentation ("docs") that needs adding/updating and removed triage Waiting for maintainers to take a look labels Sep 16, 2021
@thw0rted
Copy link
Contributor Author

Thanks Brad. I actually originally came here to get advice on how to address the issue, then while I was writing up my help request, I found your comment (linked in the OP) with a good pattern to follow. I thought it would be useful so I "converted" the issue I had been writing to a suggestion for docs improvement, but a PR would have been better. I'll work on that now.

thw0rted added a commit to thw0rted/typescript-eslint that referenced this issue Sep 17, 2021
bradzacher added a commit that referenced this issue Sep 20, 2021
* Improve no-misused-promises docs

Fixes #3892

* formatting

Co-authored-by: Brad Zacher <brad.zacher@gmail.com>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Documentation ("docs") that needs adding/updating package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants