thorium-reader icon indicating copy to clipboard operation
thorium-reader copied to clipboard

fix: fix floating-promises errors

Open akx opened this issue 10 months ago • 5 comments

While viewing this code, I noticed there were a handful of places where promises were left floating – for instance, in the LCP passphrase save code, the code never actually waited for the write to go through (or whether it failed), which could be problematic.

I noticed the ESlint rule for the issue had been turned off, so I turned it on and fixed the issues – either by awaiting where possible, or voiding the promises, so as to mark them "yes, we mean to ignore the return value of this promise".

akx avatar Mar 19 '25 15:03 akx

Generally-speaking, non-awaited Promises are "code smell" and there should be ESLint rules to check this.

Regardless of whether or not a Promise/async-function has a return value, it should be awaited unless of course the Promise itself is fed into a subsequent process that is designed to consume it.

...there is also the whole other issue of try/catch error handling on rejected Promises, function coloring and drilling error states up into the call chain (which in the case of Thorium involves Redux Saga yield'ed functions).

danielweck avatar Mar 23 '25 10:03 danielweck

Generally-speaking, non-awaited Promises are "code smell" and there should be ESLint rules to check this.

Indeed. As mentioned in the PR description, the rule had been turned off (in 8cf571e69239bba6009f03b47e1c72924a934144, 2023, to be precise). It's now on.

Regardless of whether or not a Promise/async-function has a return value, it should be awaited unless of course the Promise itself is fed into a subsequent process that is designed to consume it.

There can be merit to awaiting the promise within a function instead of returning the unawaited promise (namely, more localized stack traces should the promise be rejected).

akx avatar Mar 23 '25 16:03 akx

@danielweck Rebased on current develop. There were 6 new issues I fixed.

akx avatar Aug 07 '25 11:08 akx

thank you for keeping this PR alive, much appreciated

danielweck avatar Aug 07 '25 13:08 danielweck

@danielweck Might be a good idea to merge it too! 😉

akx avatar Aug 07 '25 14:08 akx