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: add suggestion for no-return-await #16637
feat: add suggestion for no-return-await #16637
Conversation
Suggestions will be different for different errors, so a constant object won't work anymore
|
Name | Link |
---|---|
6160c18 | |
https://app.netlify.com/sites/docs-eslint/deploys/639c57a13843ef000891087a |
Please sign EasyCLA (Contributor License Agreement). |
@amareshsm just did :) |
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
lib/rules/no-return-await.js
Outdated
{ | ||
messageId: "removeAwait", | ||
fix(fixer) { | ||
const areAwaitAndAwaitedExpressionOnTheSameLine = node.loc.start.line === node.argument.loc.start.line; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can safely fix the code if the argument
is parenthesized and the opening parenthesis is on the same line as await
.
For example:
async () => {
return await (
foo()
)
};
In this example, however, node
and node.argument
do not start on the same line, so per the current condition the suggestion will not be provided.
We could do something like this:
const awaitToken = sourceCode.getFirstToken(node);
const tokenAfterAwait = sourceCode.getTokenAfter(awaitToken);
const areAwaitAndAwaitedExpressionOnTheSameLine = awaitToken.loc.start.line === tokenAfterAwait.loc.start.line;
(sourceCode
is now needed in multiple places, so we could declare const sourceCode = context.getSourceCode();
in create(context)
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to implement this, though I would actually recommend not to do it, as it increases the complexity, risks of potentially introducing errors when running the suggestion if we missed a special case, and because suggestion from my point of view don't need to cover all use cases.
I'm a bit torn, though, as this might be a relevant case in a situation like the following:
async () => {
return await (
await fetch("https://really-long-domain.com/with-an-even-longer-path")
).json()
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's fine to implement this, it isn't overly complex and we have similar logic in other rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that getTokenAfter
is not getting the token immediately after the note:
- For
async () => await bar()
, it seems thatsourceCode.getTokenAfter(awaitToken)
isnull
. - For
async function foo() { return await new Promise(resolve => { resolve(5); }); }
getTokenAfter
gives the semicolon 2 lines after theawait
, which leads to missing a suggestion that could happen.
Instead, this worked:
const [awaitToken, tokenAfterAwait] = sourceCode.getTokens(node);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For
async () => await bar()
, it seems thatsourceCode.getTokenAfter(awaitToken)
isnull
.For
async function foo() { return await new Promise(resolve => { resolve(5); }); }
getTokenAfter
gives the semicolon 2 lines after theawait
, which leads to missing a suggestion that could happen.
This looks like the result of sourceCode.getTokenAfter(node)
. Did you try this, it should work:
const awaitToken = sourceCode.getFirstToken(node);
const tokenAfterAwait = sourceCode.getTokenAfter(awaitToken);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or this:
const [awaitToken, tokenAfterAwait] = sourceCode.getFirstTokens(node, 2);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Yes, I indeed tried to getTokenAfter(node)
Looking at the alternatives now, I actually feel like the current solution with getTokens
reads cleanest and would keep it - what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sourceCode.getTokens(node)
would go from the start till the end of the node to create and return an array with all tokens of the node, while we need only the first two. If you prefer a solution with getting all the tokens we need at once, sourceCode.getFirstTokens(node, 2)
would be more optimal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I'll change it right away
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks! I'll leave this open until today's release in case anyone else wants to review it before merging.
Thanks, it was a pleasure! In case working with me wasn't too much of a hassle for you, and you have an issue that you would like me to work on, I can take one additional PR sometime around Christmas this year :) |
Merged, thanks for contributing! You can always check issues labeled accepted. For more details: https://eslint.org/docs/latest/developer-guide/contributing/working-on-issues |
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [eslint](https://eslint.org) ([source](https://github.com/eslint/eslint)) | devDependencies | minor | [`8.29.0` -> `8.30.0`](https://renovatebot.com/diffs/npm/eslint/8.29.0/8.30.0) | --- ### Release Notes <details> <summary>eslint/eslint</summary> ### [`v8.30.0`](https://github.com/eslint/eslint/releases/tag/v8.30.0) [Compare Source](eslint/eslint@v8.29.0...v8.30.0) #### Features - [`075ef2c`](eslint/eslint@075ef2c) feat: add suggestion for no-return-await ([#​16637](eslint/eslint#16637)) (Daniel Bartholomae) - [`7190d98`](eslint/eslint@7190d98) feat: update globals ([#​16654](eslint/eslint#16654)) (Sébastien Règne) #### Bug Fixes - [`1a327aa`](eslint/eslint@1a327aa) fix: Ensure flat config unignores work consistently like eslintrc ([#​16579](eslint/eslint#16579)) (Nicholas C. Zakas) - [`9b8bb72`](eslint/eslint@9b8bb72) fix: autofix recursive functions in no-var ([#​16611](eslint/eslint#16611)) (Milos Djermanovic) #### Documentation - [`6a8cd94`](eslint/eslint@6a8cd94) docs: Clarify Discord info in issue template config ([#​16663](eslint/eslint#16663)) (Nicholas C. Zakas) - [`ad44344`](eslint/eslint@ad44344) docs: CLI documentation standardization ([#​16563](eslint/eslint#16563)) (Ben Perlmutter) - [`293573e`](eslint/eslint@293573e) docs: fix broken line numbers ([#​16606](eslint/eslint#16606)) (Sam Chen) - [`fa2c64b`](eslint/eslint@fa2c64b) docs: use relative links for internal links ([#​16631](eslint/eslint#16631)) (Percy Ma) - [`75276c9`](eslint/eslint@75276c9) docs: reorder options in no-unused-vars ([#​16625](eslint/eslint#16625)) (Milos Djermanovic) - [`7276fe5`](eslint/eslint@7276fe5) docs: Fix anchor in URL ([#​16628](eslint/eslint#16628)) (Karl Horky) - [`6bef135`](eslint/eslint@6bef135) docs: don't apply layouts to html formatter example ([#​16591](eslint/eslint#16591)) (Tanuj Kanti) - [`dfc7ec1`](eslint/eslint@dfc7ec1) docs: Formatters page updates ([#​16566](eslint/eslint#16566)) (Ben Perlmutter) - [`8ba124c`](eslint/eslint@8ba124c) docs: update the `prefer-const` example ([#​16607](eslint/eslint#16607)) (Pavel) - [`e6cb05a`](eslint/eslint@e6cb05a) docs: fix css leaking ([#​16603](eslint/eslint#16603)) (Sam Chen) #### Chores - [`f2c4737`](eslint/eslint@f2c4737) chore: upgrade [@​eslint/eslintrc](https://github.com/eslint/eslintrc)[@​1](https://github.com/1).4.0 ([#​16675](eslint/eslint#16675)) (Milos Djermanovic) - [`ba74253`](eslint/eslint@ba74253) chore: standardize npm script names per [#​14827](eslint/eslint#14827) ([#​16315](eslint/eslint#16315)) (Patrick McElhaney) - [`0d9af4c`](eslint/eslint@0d9af4c) ci: fix npm v9 problem with `file:` ([#​16664](eslint/eslint#16664)) (Milos Djermanovic) - [`90c9219`](eslint/eslint@90c9219) refactor: migrate off deprecated function-style rules in all tests ([#​16618](eslint/eslint#16618)) (Bryan Mishkin) </details> --- ### Configuration📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC43MC40IiwidXBkYXRlZEluVmVyIjoiMzQuNzAuNCJ9--> Co-authored-by: cabr2-bot <cabr2.help@gmail.com> Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1689 Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org> Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org> Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Prerequisites checklist
What is the purpose of this pull request? (put an "X" next to an item)
[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[X] Other, please explain:
Add suggestion to
no-return-await
, see #16632What changes did you make? (Give an overview)
I've added a suggestion for removing
await
beforereturn
to the ruleno-return-await
and adapted the tests for all failing cases to ensure that the correct suggestion is generated.Is there anything you'd like reviewers to focus on?
The change itself should be straight forward. I'm unsure if the
createErrorList
function I added in the test follows the code style used in this repo, though, and if it doesn't, am happy about suggestions how to do it differently.