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

feat!: use fetch #365

Merged
merged 38 commits into from
Feb 11, 2024
Merged

feat!: use fetch #365

merged 38 commits into from
Feb 11, 2024

Conversation

merceyz
Copy link
Member

@merceyz merceyz commented Jan 28, 2024

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 #361
FIxes #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.

sources/fetchUtils.ts Outdated Show resolved Hide resolved
sources/fetchUtils.ts Outdated Show resolved Hide resolved
@merceyz merceyz marked this pull request as draft February 3, 2024 22:09
.gitattributes Outdated Show resolved Hide resolved
tests/recordRequests.js Outdated Show resolved Hide resolved
tests/recordRequests.js Outdated Show resolved Hide resolved
tests/recordRequests.js Outdated Show resolved Hide resolved
@aduh95
Copy link
Contributor

aduh95 commented Feb 3, 2024

Worth noting: nock just added support for fetch in their last beta release: nock/nock#2580

@merceyz
Copy link
Member Author

merceyz commented Feb 4, 2024

@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
Copy link
Contributor

stduhpf commented Feb 4, 2024

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
Copy link
Contributor

aduh95 commented Feb 4, 2024

I can confirm @stduhpf findings 👍

@aduh95
Copy link
Contributor

aduh95 commented Feb 4, 2024

Unable to deserialize cloned data again, quite weird..

@merceyz
Copy link
Member Author

merceyz commented Feb 4, 2024

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
Copy link
Contributor

aduh95 commented Feb 5, 2024

Yeah but what’s weird is that we were able to reproduce the failure of 0643456 but on later commits, the tests pass locally with NOCK_ENV=replay 🤷‍♂️

@merceyz
Copy link
Member Author

merceyz commented Feb 7, 2024

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

@merceyz merceyz marked this pull request as ready for review February 7, 2024 19:03
@merceyz merceyz requested a review from aduh95 February 7, 2024 23:23
sources/npmRegistryUtils.ts Outdated Show resolved Hide resolved
tests/recordRequests.js Show resolved Hide resolved
@merceyz merceyz requested a review from aduh95 February 11, 2024 15:23
sources/corepackUtils.ts Outdated Show resolved Hide resolved
sources/httpUtils.ts Outdated Show resolved Hide resolved
sources/httpUtils.ts Outdated Show resolved Hide resolved
sources/httpUtils.ts Outdated Show resolved Hide resolved
@aduh95 aduh95 merged commit fe6a307 into nodejs:main Feb 11, 2024
10 checks passed
@merceyz merceyz deleted the merceyz/refactor/fetch branch February 11, 2024 23:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[feature request] support fetch COREPACK_NPM_REGISTRY from http protocol REGISTRY
5 participants