generator-jhipster
generator-jhipster copied to clipboard
Adjust async/await usage in createAsyncThunk for payloadCreator functions
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
!paginationInfiniteScrollcondition 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.jsonand confirmed that no errors are raised when runningnpm run lint. - I confirmed that there are no differences in the results of
npm run e2e:cypressbefore and after making these changes.
Please make sure the below checklist is followed for Pull Requests.
- [x] All continuous integration tests are green
- [ ] Tests are added where necessary
- [ ] The JDL part is updated if necessary
- [ ] jhipster-online is updated if necessary
- [ ] Documentation is added/updated where necessary
- [X] Coding Rules & Commit Guidelines as per our CONTRIBUTING.md document are followed
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.
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, 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"]
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/
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.
I want to know @qmonmert opinion. I think we had similar code in the past.
(Note: I have not committed
.eslintrc.jsonas it would affect other components ofcreateAsyncThunk, 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.
Personally I am against return await 🙂 this guy convinced me https://youtu.be/Pz2cL01bmwQ?t=774
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 😄.