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

Fix unresolved promises #3138

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

Fix unresolved promises #3138

wants to merge 7 commits into from

Conversation

davidanthoff
Copy link
Member

@davidanthoff davidanthoff commented Oct 26, 2022

We had a gazillion unresolved promises in the codebase where we essentially didn't have the necessary await in the code. I now enabled some eslint rules to identify those situations, and then fixed them.

We can probably expect a bunch of new crash reports from this, previously we called a lot of functions in a fire-and-forget mode where any exception just disappears into the nothing :)

TODO

  • Check all instances of then and catch.
  • Figure out how events interact with promises.

@@ -64,12 +64,24 @@ export function inferJuliaNumThreads(): string {
* Same as `vscode.commands.registerCommand`, but with added middleware.
* Currently sends any uncaught errors in the command to crash reporting.
*/
export function registerCommand(cmd: string, f) {
const fWrapped = (...args) => {
export function registerAsyncCommand<T1 extends unknown[],T2>(cmd: string, f: (...arg: T1) => Promise<T2>) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thoughts on only using one function here which explicitly async-ifies everything?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also added an eslint rule that errors if a function is async when it isn't necessary. I guess we could remove that rule, but on the other hand it does seem good to not introduce unnecessary async functions?

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

Successfully merging this pull request may close these issues.

None yet

2 participants