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

fix: custom registry with path after base url #506

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jorjao81
Copy link

when using a custom registry with a path after the base URL (like https://registry.example.org/with-path/npm) and using COREPACK_NPM_TOKEN corepack would not set the Bearer token correctly for the file download (it did set it for downloading jsons).

while fixing it I also saw that the logic for setting the bearer token was duplicated. I removed the duplicated logic, always setting the bearer in the same place and removed the strange condition for checking the origin (it is not checked when using username password, why then check when using the token?)

some additional minor changes make the code intent clearer IMHO

let me know if I missed something

when using a custom registry with a path after the base URL
(like `https://registry.example.org/with-path/npm`) and using
COREPACK_NPM_TOKEN corepack would not set the Bearer token correctly for
the file download (it did set it for downloading jsons).

while fixing it I also saw that the logic for setting the bearer token
was duplicated. I removed the duplicated logic, always setting the
bearer in the same place and removed the strange condition for checking
the origin (it is not checked when using username password, why then
check when using the token?)

some additional minor changes make the code intent clearer IMHO
@aduh95
Copy link
Contributor

aduh95 commented Jul 12, 2024

Any chance you could instead add a test that uses our custom registry? There are a few unrelated changes in the tests that makes it harder to understand what behavior is being changed

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.

2 participants