Skip to content

fix: fix floating-promises errors#2888

Draft
akx wants to merge 1 commit intoedrlab:developfrom
akx:floating-promises
Draft

fix: fix floating-promises errors#2888
akx wants to merge 1 commit intoedrlab:developfrom
akx:floating-promises

Conversation

@akx
Copy link
Copy Markdown
Contributor

@akx akx commented Mar 19, 2025

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


// flush session because user ask to read a publication
flushSession();
await flushSession();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

flushSession doesn't need to be async i guess

export const flushSession = async () => {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Making it non-async feels out of scope for this particular PR.

However, it being async doesn't matter – it acts like a synchronous function since there are no continuation points (awaits) within.

@danielweck
Copy link
Copy Markdown
Member

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

@akx akx force-pushed the floating-promises branch from 68369e6 to 923ff21 Compare March 23, 2025 16:03
@akx
Copy link
Copy Markdown
Contributor Author

akx commented Mar 23, 2025

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 8cf571e, 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).

@danielweck danielweck marked this pull request as draft June 14, 2025 10:13
@akx akx force-pushed the floating-promises branch from 923ff21 to 11b8ac9 Compare August 7, 2025 11:34
@akx akx force-pushed the floating-promises branch from 11b8ac9 to 0f9a3b8 Compare August 7, 2025 11:40
@akx akx marked this pull request as ready for review August 7, 2025 11:40
@akx
Copy link
Copy Markdown
Contributor Author

akx commented Aug 7, 2025

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

@danielweck
Copy link
Copy Markdown
Member

thank you for keeping this PR alive, much appreciated

@akx
Copy link
Copy Markdown
Contributor Author

akx commented Aug 7, 2025

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

@danielweck danielweck marked this pull request as draft October 6, 2025 10:36
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.

3 participants