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: download registry #219

Merged
merged 4 commits into from
Feb 17, 2023
Merged

fix: download registry #219

merged 4 commits into from
Feb 17, 2023

Conversation

xiaoxiangmoe
Copy link
Contributor

@xiaoxiangmoe xiaoxiangmoe commented Dec 30, 2022

Fix download registry.


Enviroment:

  • corepack 0.15.2
  • node 19.3.0

Bug description:

In China, access to https://registry.npmjs.org,https://registry.yarnpkg.com/ and https://repo.yarnpkg.com/ is unstable. We should use https://registry.npmmirror.com instead.

We can modify our /etc/host file add a line 127.0.0.1 registry.npmjs.org to simulate npm being unreachable.

Then run COREPACK_NPM_REGISTRY="https://registry.npmmirror.com" corepack prepare pnpm@7.17.0 --activate. It will try to download https://registry.npmjs.org/pnpm/-/pnpm-7.17.0.tgz. COREPACK_NPM_REGISTRY is not working.

@xiaoxiangmoe
Copy link
Contributor Author

@micsco @aduh95 This PR is releated to #186

@xiaoxiangmoe
Copy link
Contributor Author

xiaoxiangmoe commented Dec 30, 2022

This PR works for npm and pnpm because the download links for them are start with https://registry.npmjs.org/

definitions.npm.ranges['*'].url:

"url": "https://registry.npmjs.org/npm/-/npm-{}.tgz",

definitions.pnpm.ranges['>=6.0.0'].url:
"url": "https://registry.npmjs.org/pnpm/-/pnpm-{}.tgz",


But it seems not work for yarn.
definitions.yarn.ranges['<2.0.0'].url:

"url": "https://registry.yarnpkg.com/yarn/-/yarn-{}.tgz",

definitions.yarn.ranges['>=2.0.0'].url:
"url": "https://repo.yarnpkg.com/{}/packages/yarnpkg-cli/bin/yarn.js",


@arcanis Should we change the yarn download link in config.json to download from npm registry?

sources/corepackUtils.ts Outdated Show resolved Hide resolved
@aduh95
Copy link
Contributor

aduh95 commented Jan 27, 2023

Should we change the yarn download link in config.json to download from npm registry?

AFAIK Yarn 2+ versions are no longer published on the npm registry1 (the last 2+ version published was 2.4.3 published in September 2021), using an npm mirror wouldn't work. I'm not sure what would be the best way to make that customizable, adding a COREPACK_YARN_REGISTRY would not be a great idea, as if more package managers decide to use a custom registry it's going to be unmaintainable.

Footnotes

  1. https://www.npmjs.com/package/yarn?activeTab=versions

@arcanis
Copy link
Contributor

arcanis commented Jan 27, 2023

Perhaps we could have a generic COREPACK_DNS_OVERRIDE=registry.something.com/registry.somethingelse.com which would replace all calls to the first domain by calls to the second? This would work with both npm and custom registries.

@xiaoxiangmoe
Copy link
Contributor Author

@arcanis @aduh95 Do you have any idea about how to use COREPACK_DNS_OVERRIDE to config both npm registery and yarn registery?

@arcanis
Copy link
Contributor

arcanis commented Jan 27, 2023

Perhaps, to ensure the syntax is somewhat familiar, we can make it a JSON object? For example:

COREPACK_DNS_OVERRIDE='{
  "registry.yarnpkg.com": "mirror.something.com",
  "registry.npmjs.com": "mirror.somethingelse.com"
}' yarn

Main problem are the newlines which, while they seem to not be an actual problem when use in env variables per se, may cause wonky behaviours on scripts making incorrect assumptions about the result of $ env.

Alternatively, perhaps we could have some corepack settings in the closest package.json, since we already know its location to extract the package manager to use.

@aduh95
Copy link
Contributor

aduh95 commented Jan 27, 2023

Main problem are the newlines

Worth noting that JSON does not require using new lines, so if the new lines are a problem, a simple workaround is to not use them.

Alternatively, perhaps we could have some corepack settings in the closest package.json, since we already know its location to extract the package manager to use.

I don't think it's a good idea, a DNS override is not something that's defined on a project basis, but rather as a system level, so env variable would make the most sense. Or am I missing something?

@xiaoxiangmoe
Copy link
Contributor Author

xiaoxiangmoe commented Feb 10, 2023

@aduh95 @arcanis Do you have any idea about new env variable for this featurer to replacing COREPACK_DNS_OVERRIDE?

sources/corepackUtils.ts Outdated Show resolved Hide resolved
@aduh95 aduh95 merged commit 1b35362 into nodejs:main Feb 17, 2023
@aduh95
Copy link
Contributor

aduh95 commented Feb 17, 2023

@xiaoxiangmoe if you want to make COREPACK_DNS_OVERRIDE happen, that'd be awesome! In the mean time, let's merge this and release so it can already unblock users of npm and pnpm.

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.

3 participants