-
Notifications
You must be signed in to change notification settings - Fork 173
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: add support for configurable registries and applicable authentication options #186
Conversation
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.
Could you do this without using npm-registry-fetch
?
This PR increases the bundle size by 1MB and startup time by about 82% and that seems a bit excessive for essentially adding a few headers.
$ hyperfine "node ./dist-before/corepack.js --version" "node ./dist-after/corepack.js --version"
Benchmark 1: node ./dist-before/corepack.js --version
Time (mean ± σ): 49.7 ms ± 0.6 ms [User: 47.4 ms, System: 3.6 ms]
Range (min … max): 48.9 ms … 52.0 ms 57 runs
Benchmark 2: node ./dist-after/corepack.js --version
Time (mean ± σ): 90.7 ms ± 1.5 ms [User: 83.9 ms, System: 8.3 ms]
Range (min … max): 89.2 ms … 95.1 ms 33 runs
Summary
'node ./dist-before/corepack.js --version' ran
1.82 ± 0.04 times faster than 'node ./dist-after/corepack.js --version'
9c12880
to
7e61a46
Compare
@merceyz - Per your suggestion, I removed the dependencies and manually added the appropriate headers. Now I see the following results when benchmarking: $ hyperfine "node ./dist-before/corepack.js --version" "node ./dist-after/corepack.js --version"
Benchmark 1: node ./dist-before/corepack.js --version
Time (mean ± σ): 55.9 ms ± 2.1 ms [User: 49.8 ms, System: 7.8 ms]
Range (min … max): 53.9 ms … 65.2 ms 47 runs
Benchmark 2: node ./dist-after/corepack.js --version
Time (mean ± σ): 56.2 ms ± 2.1 ms [User: 49.1 ms, System: 8.8 ms]
Range (min … max): 53.5 ms … 62.2 ms 54 runs
Summary
'node ./dist-before/corepack.js --version' ran
1.00 ± 0.05 times faster than 'node ./dist-after/corepack.js --version' |
This PR helps me a lot. |
7e61a46
to
1629428
Compare
Thanks, @aduh95 - Good catch on the password encoding process. |
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.
LGTM modulo a few unrelated empty lines that were added probably by accident.
@micsco it looks like the added test fails:
Can you take a look? |
@aduh95 apologies, I used the Username env var twice, instead of using the password - thankfully tests caught it but I forgot to check. Have updated the branch. |
tests/npmRegistryUtils.test.ts
Outdated
it(`throw usage error if COREPACK_ENABLE_NETWORK env is set to 0`, async () => { | ||
process.env.COREPACK_ENABLE_NETWORK = `0`; | ||
|
||
expect(fetchAsJson(`package-name`)).rejects.toThrowError(); |
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.
expect(fetchAsJson(`package-name`)).rejects.toThrowError(); | |
await expect(fetchAsJson(`package-name`)).rejects.toThrowError(); |
Could you update the description to mention that this PR |
0627c63
to
e2696a0
Compare
Thanks for the comprehensive review @merceyz - I've applied all your changes and updated the PR. |
e2696a0
to
3865496
Compare
This adds support per #66
Resolves #66