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

clients/viem: use interval for routine fetching, unref intervalId #383

Merged
merged 4 commits into from
Sep 10, 2024

Conversation

CedarMist
Copy link
Member

@CedarMist CedarMist commented Sep 6, 2024

Provide actual fix for problem inside #379

@CedarMist CedarMist self-assigned this Sep 6, 2024
@CedarMist CedarMist requested a review from aefhm September 6, 2024 12:55
Copy link

netlify bot commented Sep 6, 2024

Deploy Preview for oasisprotocol-sapphire-paratime ready!

Name Link
🔨 Latest commit c34b198
🔍 Latest deploy log https://app.netlify.com/sites/oasisprotocol-sapphire-paratime/deploys/66e0ab19903be200092e8191
😎 Deploy Preview https://deploy-preview-383--oasisprotocol-sapphire-paratime.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@CedarMist CedarMist added the javascript Pull requests that update JavaScript code label Sep 6, 2024
@CedarMist CedarMist added this to the integrations-viem-2.0.1 milestone Sep 6, 2024
@CedarMist
Copy link
Member Author

CedarMist commented Sep 6, 2024

This won't work in browsers.

We can do something like:

if( typeof x === 'object' && 'unref' in x && typeof x.unref == 'function' )

or just

if( typeof x.unref === 'function' )

?

Likewise we're not doing any in-browser testing for packages which will primarily be used in browsers, so none of this would've been caught in CI.

@CedarMist
Copy link
Member Author

Will find a way of linting the TS code for NodeJS vs WebBrowser compatibility.

@CedarMist CedarMist force-pushed the CedarMist/viem-node-hangs branch from c0294a6 to 83d5ae5 Compare September 6, 2024 17:05
@CedarMist
Copy link
Member Author

Have logged a ticket/discussion with Viem w.r.t async serializers, which would entirely avoid this hacky workaround. I should've done this earlier.

wevm/viem#2696

@CedarMist CedarMist marked this pull request as ready for review September 9, 2024 00:37
@aefhm
Copy link
Contributor

aefhm commented Sep 9, 2024

A standalone NodeJS script reproduces the exit error pretty easily, but reproducing it in vitest has been tricky. Test harness is not waiting for the Node process, and does not hang which is the behavior we want to reproduce in the failing case. We could use a test script and assert that this exits within 1000 milliseconds though.

const transport = sapphireHttpTransport();
const chain = sapphireLocalnet;
const publicClient = createPublicClient({ chain, transport });
defineChain({
    id: 0x5afd,
    name: "Oasis Sapphire Localnet",
    network: "sapphire-localnet",
    nativeCurrency: { name: "Sapphire Local Rose", symbol: "TEST", decimals: 18 },
    rpcUrls: {
        default: {
            http: ["http://localhost:8545"],
        },
    },
    serializers: {
        transaction: createSapphireSerializer(publicClient)
    },
    testnet: true,
});

@CedarMist
Copy link
Member Author

CedarMist commented Sep 10, 2024

Ok, I can reproduce this.
It appears that my unref fix works correctly. I will see about how to add a test.

Have added a timeout test, to verify that your example script exits after 2 seconds.

@aefhm aefhm force-pushed the CedarMist/viem-node-hangs branch from 9d987c4 to c34b198 Compare September 10, 2024 20:24
@CedarMist CedarMist merged commit 87bba83 into main Sep 10, 2024
10 checks passed
@CedarMist CedarMist deleted the CedarMist/viem-node-hangs branch September 10, 2024 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug client javascript Pull requests that update JavaScript code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants