-
Notifications
You must be signed in to change notification settings - Fork 20
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
Feat/rpc handler #19
Feat/rpc handler #19
Conversation
aaa912f
to
fc02db1
Compare
Tests seem to works but some of them do not exit properly, I suspect some import containing async calls that is not awaited, see here. |
running |
Co-authored-by: Mentlegen <9807008+gentlementlegen@users.noreply.github.com>
@Keyrxng A common scenario I encountered is when you import a file ( run().then().catch(e => {}) This mean on import, async functions run instantly but are not awaited and run beyond the tear down of Jest, if that helps you. |
@gentlementlegen I understand what you mean and why that would be the case but there isn't anything like that in either of the two codebases in this case I've just had a look through them. Theres Unless the I think it's directly related to the racing of the RPCs from what I've seen but like I said I've not been able to remove the warning 100% of the time You don't see the error in pay.ubq.fi tests only in jest tests do we see it. Using |
@@ -0,0 +1,26 @@ | |||
// @ts-expect-error - no types |
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.
rpc-handler
seems to have types?
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.
The repo does but the package does not for some reason. My test pkg I published has types and it's the same flow that was used to publish but the UBQ pkg doesn't have types in either of the versions on npmjs
https://www.npmjs.com/package/@ubiquity-dao/rpc-handler?activeTab=code
the Jest has detected the following 2 open handles potentially keeping Jest from exiting:
● TLSWRAP
16423 | withCredentials
16424 | });
> 16425 | let response = await fetch(request);
| ^
16426 | const isStreamResponse = supportsResponseStream && (responseType ===
"stream" || responseType === "response");
16427 | if (supportsResponseStream && (onDownloadProgress || isStreamResponse)) {
16428 | const options = {};
``` |
@Keyrxng Could you then try to:
|
@gentlementlegen I've mocked the handler and return a valid provider instead of racing them. Tests could be refactored to use a local anvil instance like other projects that rely on on-chain interactions if this impl is a problematic approach to tests. The warning is no longer present so the issue stems from |
I think this should be solved, because at the moment you just get a warning that some code is running after Jest tear down, but if that code was actually outputting things using |
This reverts commit 91470cc.
@rndquu Wrote the spec and approved the pull so I think it's an automatic accept @gentlementlegen We should consider this for the auto pull merge plugin |
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.
Implicit ok
Opened #29 |
Resolves #18
Requires #17