search

nodejs / corepack

Zero-runtime-dependency package acting as bridge between Node projects and their package managers
MIT License
2.52k stars 165 forks source link

feat!: use `fetch` #365

Closed merceyz closed 7 months ago

merceyz commented 8 months ago

Changes Corepack to use the built-in fetch and the ProxyAgent from Undici.

Since fetch supports compressed responses by default the bandwidth used when fetching from https://repo.yarnpkg.com/tags is also reduced (Transferred 1.71 kB (5.46 kB size).

Closes https://github.com/nodejs/corepack/pull/361 FIxes https://github.com/nodejs/corepack/issues/293

$ du dist/lib/corepack*
956     dist/lib/corepack-new.cjs
2272    dist/lib/corepack-old.cjs
$ node -v
v20.11.0
$ hyperfine -w 5 "node ./dist/lib/corepack-new.cjs --version" "node ./dist/lib/corepack-old.cjs --version"
Benchmark 1: node ./dist/lib/corepack-new.cjs --version
  Time (mean ± σ):      47.6 ms ±   0.9 ms    [User: 43.6 ms, System: 6.4 ms]
  Range (min … max):    46.0 ms …  50.7 ms    63 runs

Benchmark 2: node ./dist/lib/corepack-old.cjs --version
  Time (mean ± σ):      61.1 ms ±   1.2 ms    [User: 53.6 ms, System: 10.0 ms]
  Range (min … max):    59.5 ms …  64.7 ms    50 runs

Summary
  node ./dist/lib/corepack-new.cjs --version ran
    1.28 ± 0.03 times faster than node ./dist/lib/corepack-old.cjs --version

BREAKING CHANGE: Corepack is now using the built-in fetch method and the ProxyAgent from Undici to perform requests, setups using custom registries and/or proxies might be affected.

aduh95 commented 8 months ago

Worth noting: nock just added support for fetch in their last beta release: https://github.com/nock/nock/pull/2580

merceyz commented 8 months ago

@aduh95 any suggestions for how to debug the Windows issue?

I've tried storing body as a Buffer and Uint8Array and it throws Unable to deserialize cloned data for both and I can't reproduce it locally.

stduhpf commented 8 months ago

I was able to reproduce the ci failure on my machine. This patch fixed it for me: 0001-Use-ArrayBuffer-instead-of-Uint8Array.patch

aduh95 commented 8 months ago

I can confirm @stduhpf findings :+1:

aduh95 commented 8 months ago

Unable to deserialize cloned data again, quite weird..

merceyz commented 8 months ago

Yeah, I already tried using an ArrayBuffer and the Map shouldn't be a problem since it worked when body was a string, but it was worth a try.

aduh95 commented 8 months ago

Yeah but what’s weird is that we were able to reproduce the failure of https://github.com/nodejs/corepack/pull/365/commits/064345669de97413e106cb84141d3180c47fbe0f but on later commits, the tests pass locally with NOCK_ENV=replay 🤷‍♂️

merceyz commented 8 months ago

@aduh95 @stduhpf since you're able to reproduce the issue locally could you make a bug report upstream to Node.js and/or v8?