build: resolve all floating promise warnings #7465
Conversation
samsiegart
force-pushed
the
floating-promise
branch
2 times, most recently
from
f31d865 to
1fc7cb5
Compare
samsiegart
force-pushed
the
floating-promise
branch
2 times, most recently
from
76b4ac6 to
6774abc
Compare
|
Some packages, like SwingSet and swingset-runner, seem to get stuck on |
Maybe due to cycles. Try restricting the linting to subpaths to see what report you can get without hanging. |
I ran again with Before looking further into this, I'll note to @ivanlei that I've hit the 1-day-of-work timebox for this task. And, I have some doubts (see bottom of PR description) of the benefit of this PR. |
|
@turadg Ivan says "for stuff that stalls i think just skip them". Given that I've run the command on a per-package basis for each package that doesn't stall, I think it makes sense to merge (after review) these changes at least. |
Fair enough. Please update the description so that it doesn't close the ticket to complete the work. (I was going to say that anyway since we shouldn't close the ticket, an audit, until just before release since the code is still changing.) |
Done |
There was a problem hiding this comment.
I ran into a failure today in zcfZygote because a promise wasn't awaited. I see in here it's marked void.
I notice not all promises here are voided but most of them are, so I suspect there are other voids that really should be awaited or error handled. Can you trim down the changes to files you're confident in the way the promises are handled?
| @@ -361,7 +361,7 @@ export const makeZCFZygote = async ( | |||
| privateArgs = undefined, | |||
| ) => { | |||
| zoeInstanceAdmin = instanceAdminFromZoe; | |||
| initSeatMgrAndMintFactory(); | |||
| void initSeatMgrAndMintFactory(); | |||
There was a problem hiding this comment.
this should be awaited. if it fails the contract should not restart
| @@ -417,7 +417,7 @@ export const makeZCFZygote = async ( | |||
| prepare || Fail`prepare must be defined to upgrade a contract`; | |||
| zoeInstanceAdmin = zcfBaggage.get('zcfInstanceAdmin'); | |||
| instanceRecHolder = zcfBaggage.get('instanceRecHolder'); | |||
| initSeatMgrAndMintFactory(); | |||
| void initSeatMgrAndMintFactory(); | |||
samsiegart
force-pushed
the
floating-promise
branch
from
6774abc to
cd3a199
Compare
|
Having some trouble with commit history so started a new PR for just the changes I'm confident about #7485 |
refs: #6000
Description
Resolve all violations of
@typescript-eslint/no-floating-promisesin all packages (except forSwingSetandswingset-runner, see discussion below).Change rule from
warntoerrorto ensure more instances don't come up after this effort.Security Considerations
N/A
Scaling Considerations
N/A
Documentation Considerations
N/A
Testing Considerations
Ran
NODE_OPTIONS='--max-old-space-size=8192' AGORIC_ESLINT_TYPES=true yarn lint:eslintin each package.Unit tests should still pass.
Anything with a change to add explicit
voidwill not change any behavior. However, it's possible that there are still hidden bugs resulting from not awaiting or catching these promises. Is it in our best interest to usevoidanywhere then? By doing so, we risk sweeping those bugs under the rug, despite appeasing the lint rule. If we must remove allvoid, then this is a much larger task.