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

Adjust async/await usage in createAsyncThunk for payloadCreator functions #22452

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

hide212131
Copy link
Contributor

@hide212131 hide212131 commented Jun 7, 2023

Resolves #19133

The payloadCreator in createAsyncThunk has been adjusted according to the following policy. If there are any issues, please let me know and I will promptly make corrections!

  • Generally, it has been changed to async () => {... await axios…} for the sake of consistency, to reduce the risk of errors during future code modifications, and to align with the common practice seen in createAsyncThunk.
  • However, applying this to 'expression body' parts contradicts the intention to keep things simple. Therefore, in these cases, async/await has been removed.
  • Regarding @mshima's suggestion to consolidate expressions, I consciously chose not to. This is because I believe the following code generated under the !paginationInfiniteScroll condition is redundant:
    const result = await axios.post<...>...;
    return result;
    

After making these adjustments, I generated the react-default sample and confirmed the following:

  • For testing purposes only (not committed), I added "require-await": "error" to .eslintrc.json and confirmed that no errors are raised when running npm run lint.
  • I confirmed that there are no differences in the results of npm run e2e:cypress before and after making these changes.

Please make sure the below checklist is followed for Pull Requests.

When you are still working on the PR, consider converting it to Draft (below reviewers) and adding skip-ci label, you can still see CI build result at your branch.

@mshima
Copy link
Member

mshima commented Jun 9, 2023

return await is not a good practice:
https://eslint.org/docs/latest/rules/no-return-await

If a promise is returned, the method is recommended to be async (avoids having to both catch errors and catch rejections):
https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/docs/rules/promise-function-async.md

@hide212131
Copy link
Contributor Author

@mshima, thank you for your response amidst the busy preparations for the v8 release!

Since your comment, I have been investigating the 'no-return-await' and 'return-await' issues, among others. The conversation is indeed complex with various intertwined opinions...

One thing I found is that 'no-return-await' was removed from JavaScript Standard Style around 2019 due to concerns over 1) stack traces and 2) overlooked async function calls.
standard/standard#1442
I lean towards the view that both of these considerations (1 & 2) might be more appropriate in the context of JHipster.

At this juncture, it might be beneficial to consider adding new rules to .eslintrc.json, such as:

    "no-return-await": "off",
    "@typescript-eslint/return-await": ["error", "always"]

@mshima
Copy link
Member

mshima commented Jun 18, 2023

I’m ok with awaited return value.

Always having async at async () => axios.get() is recommended by https://typescript-eslint.io/rules/promise-function-async/

@hide212131 hide212131 force-pushed the issue19133-async-await-adjustment branch from 4523c15 to 149c7cd Compare June 18, 2023 09:14
@hide212131
Copy link
Contributor Author

Thank you @mshima for your comment. I agree with your point on promise-function-async; it was something I had overlooked.

I have made the necessary corrections, which, expressed in eslint terms, would look like the following rules:

"no-return-await": "off",
"@typescript-eslint/return-await": ["error", "always"],
"@typescript-eslint/promise-function-async": "error"

(Note: I have not committed .eslintrc.json as it would affect other components of createAsyncThunk, as well as Angular and Vue. This has been used only for verification.)

Please review the changes made. I have followed the Submission Guidelines and executed git rebase main -i; git push -f.

@mshima
Copy link
Member

mshima commented Jun 18, 2023

I want to know @qmonmert opinion. I think we had similar code in the past.

@mshima mshima requested a review from qmonmert June 18, 2023 14:24
@mshima
Copy link
Member

mshima commented Jun 18, 2023

(Note: I have not committed .eslintrc.json as it would affect other components of createAsyncThunk, as well as Angular and Vue. This has been used only for verification.)

Probably it’s an ejs file, so the rule can be conditional on react.

@qmonmert
Copy link
Contributor

qmonmert commented Jun 18, 2023

Personally I am against return await 🙂 this guy convinced me https://youtu.be/Pz2cL01bmwQ?t=774

@mshima
Copy link
Member

mshima commented Jun 20, 2023

Personally I am against return await 🙂 this guy convinced me https://youtu.be/Pz2cL01bmwQ?t=774

I need time to watch the video and try to understand the examples, don't understand French 😄.

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

Successfully merging this pull request may close these issues.

async/await issue with createAsyncThunk
4 participants