-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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
[eslint] prevent async Promise constructor mistakes #110349
[eslint] prevent async Promise constructor mistakes #110349
Conversation
…hidden-promise-constructor-rejections
Pinging @elastic/kibana-operations (Team:Operations) |
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.
Approving on behalf of @elastic/stack-monitoring-ui - the changes look good and might even fix some of the error handling issues we've had come up with the logstash pipeline view via SDH.
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.
Alerting changes LGTM!
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.
app services changes lgtm
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, just one note.
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.
Stack management changes LGTM!
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.
maps/*
and maps_ems/*
changes ok. thx!
…g, fixing manually
…hidden-promise-constructor-rejections
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,
- I searched for
security_solution
and saw the two areas that were fixed.
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.
Code review LGTM for @elastic/kibana-vis-editors files
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
Co-authored-by: spalger <spalger@users.noreply.github.com>
💔 Backport failed
Successful backport PRs will be merged automatically after passing CI. To backport manually run: |
Co-authored-by: spalger <spalger@users.noreply.github.com> # Conflicts: # x-pack/plugins/security_solution/server/lib/detection_engine/routes/rules/import_rules_route.ts
Co-authored-by: spalger <spalger@users.noreply.github.com> # Conflicts: # src/plugins/charts/public/services/active_cursor/use_active_cursor.test.ts # x-pack/plugins/security_solution/server/lib/detection_engine/routes/rules/import_rules_route.ts # x-pack/plugins/task_manager/server/task_scheduling.ts
…110728) * [eslint] prevent async Promise constructor mistakes (#110349) Co-authored-by: spalger <spalger@users.noreply.github.com> # Conflicts: # x-pack/plugins/security_solution/server/lib/detection_engine/routes/rules/import_rules_route.ts * autofix additional violation Co-authored-by: spalger <spalger@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
…110731) * [eslint] prevent async Promise constructor mistakes (#110349) Co-authored-by: spalger <spalger@users.noreply.github.com> # Conflicts: # src/plugins/charts/public/services/active_cursor/use_active_cursor.test.ts # x-pack/plugins/security_solution/server/lib/detection_engine/routes/rules/import_rules_route.ts # x-pack/plugins/task_manager/server/task_scheduling.ts * update yarn.lock file * add @babel/generator dependency * autofix additional violation * update yarn.lock Co-authored-by: spalger <spalger@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
After fixing #109560 I did a little searching and found many cases where we are passing async functions to the Promise constructor without handling the potential of a rejected promise being created by said async function. This PR implements a custom ESLint rule which finds such situations and fixes them by wrapping the body of the function in a
try/catch
and routing errors caught toreject()
.I don't think this produces great code, but it's an improvement over having unhandled promise rejections.
In #109565 I had manually fixed each violation and detailed some strategies for fixing this type of problem for those who are curious, but it felt like I was prescribing a code style in places and felt like we should really prevent this happening in the future with an ESLint rule. Thankfully, my work in #110161 helped me feel more confident in writing fixes like this.