generator-jhipster icon indicating copy to clipboard operation
generator-jhipster copied to clipboard

Adjust async/await usage in createAsyncThunk for payloadCreator functions

Open hide212131 opened this issue 2 years ago • 8 comments
trafficstars

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.

hide212131 avatar Jun 07 '23 16:06 hide212131

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

mshima avatar Jun 09 '23 00:06 mshima

@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. https://github.com/standard/standard/issues/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"]

hide212131 avatar Jun 09 '23 14:06 hide212131

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/

mshima avatar Jun 18 '23 01:06 mshima

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.

hide212131 avatar Jun 18 '23 09:06 hide212131

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

mshima avatar Jun 18 '23 14:06 mshima

(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.

mshima avatar Jun 18 '23 14:06 mshima

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

qmonmert avatar Jun 18 '23 15:06 qmonmert

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 😄.

mshima avatar Jun 20 '23 20:06 mshima