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

Remove cross-fetch lib as we use Node18 everywhere #10146

Open
tomasklim opened this issue Dec 1, 2023 · 7 comments
Open

Remove cross-fetch lib as we use Node18 everywhere #10146

tomasklim opened this issue Dec 1, 2023 · 7 comments
Assignees
Labels
code Code improvements dependencies Pull requests that update a dependency file

Comments

@tomasklim
Copy link
Member

tomasklim commented Dec 1, 2023

@tomasklim tomasklim added the code Code improvements label Dec 1, 2023
@tomasklim tomasklim added the dependencies Pull requests that update a dependency file label Dec 1, 2023
@komret
Copy link
Contributor

komret commented Feb 21, 2024

Removed from several packages in #11223.

Removing in from @trezor/coinjoin would break coordinatorRequest test because request fails - unlike if cross-fetch is used.

Removing it from @trezor/connect requires reworking the mocks.

@peter-sanderson
Copy link
Contributor

Lets not do this before Node19. It has some differeneces and causing troubles. See: #11820

(Details: https://satoshilabs.slack.com/archives/C02V2PSDNA2/p1711539811017599)

@github-project-automation github-project-automation bot moved this to 🤝 Needs QA in Issues Suite May 3, 2024
@github-project-automation github-project-automation bot moved this from 🤝 Needs QA to 🎯 To do in Issues Suite May 3, 2024
@peter-sanderson
Copy link
Contributor

Here is also relevant Github Issue: nodejs/node#46375

@peter-sanderson
Copy link
Contributor

Also good read: https://betterstack.com/community/guides/scaling-nodejs/nodejs-timeouts/#timing-out-a-fetch-api-request

It says that in node fetch has no timeout, but it seems that its not true

In browsers, fetch() usually times out after a set period of time which varies amongst browsers. For example, in Firefox this timeout is set to 90 seconds by default, but in Chromium, it is 300 seconds. In Node.js, no default timeout is set for fetch() requests, but the newly added AbortSignal.timeout() API provides an easy way to cancel a fetch() request when a timeout is reached.

@peter-sanderson
Copy link
Contributor

Finally another issue: nodejs/undici#1373

TBH I dont see a good solution to this right now :(

@peter-sanderson
Copy link
Contributor

peter-sanderson commented May 3, 2024

Maybe a solution is to use undici. There is a way how to define timeouts globally.

const { Agent } = require('undici');


dispatcher = new Agent({
    connectTimeout: 10 * 60e3, // 10 minutes
    bodyTimeout: 10 * 60e3, // 10 minutes
    headersTimeout: 10 * 60e3, // 10 minutes
})

globalThis[Symbol.for('undici.globalDispatcher.1')] = dispatcher


// ----------------------------------------------------------------------------

const t = Date.now()

const run = async () => {
    try {
        await fetch("https://192.168.0.1");
    } finally {
        console.log((Date.now() - t) / 1000);
    }
}

run();

Source of the idea: nodejs/undici#1373 (comment)

@komret
Copy link
Contributor

komret commented May 6, 2024

It's not pressing, let's update node first and see if it works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code Code improvements dependencies Pull requests that update a dependency file
Projects
Status: 🎯 To do
Development

Successfully merging a pull request may close this issue.

3 participants