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: add cnpm to corepack #333

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

feat: add cnpm to corepack #333

wants to merge 12 commits into from

Conversation

elrrrrrrr
Copy link

@elrrrrrrr elrrrrrrr commented Dec 5, 2023

add cnpm to corepack, closes #331

  1. ✍️ update config.json and add registry information for cnpm.
  2. 🪝 Add a post-install hook to install cnpm-related dependencies.

Fixes: #331

@elrrrrrrr elrrrrrrr changed the title add support for cnpm feat: add cnpm to corepack Dec 5, 2023
@aduh95
Copy link
Contributor

aduh95 commented Dec 5, 2023

2. 🪝 Add a post-install hook to install cnpm-related dependencies.

Could it be possible to drop that and download ready-to-be-used code instead?

config.json Outdated Show resolved Hide resolved
sources/corepackUtils.ts Outdated Show resolved Hide resolved
sources/corepackUtils.ts Outdated Show resolved Hide resolved
fengmk2 pushed a commit to cnpm/cnpm that referenced this pull request Dec 6, 2023
> 修改构建配置,将 cnpm 运行依赖进行 bundle 处理,see
nodejs/corepack#333

1. 期望 cnpm 能直接集成在 corepack 中作为 nodejs 预置的包管理器
2. 目前其他 pnpm、npm 和 yarn 都会做 bundle 处理,cnpm 由于集成了 npm,只能继续使用 bundleDeps
的方式打包

corepack 通过 tgz 进行分发, 没有时机执行相关依赖安装。

尝试过两种方式进行 bundle:
1. ❎ 通过 esbuild,无法处理 require.resolve npm 的场景,考虑到依赖稳定性需要将 npm 一并 bundle
2. ✅ 通过 bundleDeps 配置

两种方式都需要通过 `npm` mode 进行安装

目前 bundle 后 tgz 体积为 `8.7M`,和 pnpm,yarn 相比差距较大 (~2M)
.gitignore Outdated Show resolved Hide resolved
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You probably want to update sync.yml so the version of cnpm in config.json stays up-to-date

@elrrrrrrr elrrrrrrr force-pushed the cnpm branch 2 times, most recently from 229e83f to 966b3ec Compare December 7, 2023 11:40
tests/main.test.ts Outdated Show resolved Hide resolved
tests/main.test.ts Outdated Show resolved Hide resolved
@elrrrrrrr elrrrrrrr force-pushed the cnpm branch 2 times, most recently from 0999d8b to 6856b78 Compare December 8, 2023 03:12
@elrrrrrrr elrrrrrrr marked this pull request as ready for review December 8, 2023 14:42
@elrrrrrrr
Copy link
Author

Just a gentle ping 🙏🏻 @aduh95 @arcanis @merceyz
If there's anything more needed from my side, please let me know.
Eagerly awaiting your feedback

.github/workflows/sync.yml Outdated Show resolved Hide resolved
sources/nodeUtils.ts Outdated Show resolved Hide resolved
tests/main.test.ts Show resolved Hide resolved
@anonrig
Copy link
Member

anonrig commented Dec 14, 2023

cc @nodejs/security-wg

Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I confirm that cnpm is a helpful alternative to npm in China where registry.npmjs.org may be hardly reachable for common users. https://registry.npmmirror.com is a mirror of registry.npmjs.org for users based in China to get better access to npm packages.

It would definitely help users get started with Node.js and install npm dependencies in China.

I support adding cnpm and npmmirror.com to corepack.

elrrrrrrr and others added 5 commits December 14, 2023 21:39
Co-authored-by: Antoine du Hamel <[email protected]>
Co-authored-by: Antoine du Hamel <[email protected]>
Co-authored-by: Antoine du Hamel <[email protected]>
Co-authored-by: Antoine du Hamel <[email protected]>
tests/main.test.ts Outdated Show resolved Hide resolved
@joyeecheung
Copy link
Member

joyeecheung commented Dec 14, 2023

Can't review the code per-se myself, but I can also confirm that cnpm is a trust-worthy alternative primarily sponsored by Alibaba to help Node.js developers in China work around the network issues in the public network in China & create private registries within a company.

config.json Outdated Show resolved Hide resolved
Co-authored-by: Kristoffer K. <[email protected]>
@arcanis
Copy link
Contributor

arcanis commented Dec 15, 2023

I'm not familiar with cnpm, so couple of questions:

  • What makes cnpm more useful than setting a custom registry in one's configuration? Why does a whole package manager need to be built around that (rather than configure the local mirror)?

  • Does it mean that the cnpm default registry is subject to local CN legislation? Is it potentially a problem to provide that in the default Node.js installs (note that I'd have the same question if we were to change, say, pnpm to a EU mirror)?

  • Is it a wrapper around npm with a different registry? I see you implemented a similar install strategy as pnpm; are there other differencies?

@elrrrrrrr
Copy link
Author

elrrrrrrr commented Dec 15, 2023

@arcanis

Thank you for your interest and questions about cnpm. I am pleased to provide you with the following clarifications:

Why does a whole package manager need to be built around

Indeed, we offer a complete read-only mirror service capability. Developers can directly configure the registry address and use yarn, pnpm, or any package manager of their choice. However, we still provide an independent client, mainly to handle the following scenarios:

Does it mean that the cnpm default registry is subject to local CN legislation

Like all websites accessible within China, cnpm aligns with the country's internet and data regulations. For over 10 years, it has been reliably serving users while adhering to Mainland China's regulatory requirements. This compliance ensures that cnpm's default registry undergoes thorough network filing and security scanning, aligning with local policy standards. Our commitment is to provide reliable and compliant services to developers, not just in China but globally, always mindful of the legal nuances in different regions.

Are there other differencies?

cnpm includes npm to ensure maximum compatibility. In the corporate environment, we also offer some extended capabilities. We continue to advance related open-source work, including:

  • Providing faster dependency resolution. Changes on the registry side have been released, allowing the registry to directly parse semver expressions: https://registry.npmmirror.com/node/>=16 < 20
  • Offering the fastest dependency installer, rapid, available at https://github.com/cnpm/rapid. (currently beta)
  • Installing custom node runtime environments for projects through the package manager.

These features, in addition to changes on the registry side, also require client-side adaptation.

Please let me know if you have any further questions or need more information. 🙏🏻

@aladdin-add
Copy link

Whether or not to support setting registry to https://registry.npmmirror.com/ when not using cnpm?

"versions": "versions",
"tags": "dist-tags"
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably provide mentioned for the npm package, see #339

Suggested change
}
},
"npmRegistry": {
"type": "npm",
"package": "cnpm"
},

Comment on lines 5 to +8
Npm = `npm`,
Pnpm = `pnpm`,
Yarn = `yarn`,
Cnpm = `cnpm`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: ASCII/alphabetical order

Suggested change
Npm = `npm`,
Pnpm = `pnpm`,
Yarn = `yarn`,
Cnpm = `cnpm`,
Cnpm = `cnpm`,
Npm = `npm`,
Pnpm = `pnpm`,
Yarn = `yarn`,

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.

Application to Include cnpm in corepack
9 participants