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: using pnpm from corepack when install dependency #762

Merged
merged 4 commits into from
Jan 4, 2025

Conversation

OrkWard
Copy link
Contributor

@OrkWard OrkWard commented Jan 2, 2025

This PR proposes multiple improvements to the usage of package manager.

  1. corepack is bundled within node since 14.19.0, meaning that there is no need to manually install it using npm install -g corepack;
  2. Since package.json has declare packageManager here, it is supposed all dependencie should be managed by the specified manager version. Avoid install pnpm manually in ci, just run corepack enable. Meanwhile, using two package manager with different resolve algorithm will never be a good idea, for which yarn.lock is removed;
  3. Some background first: corepack works by intercepting pnpm, yarn executable in the path, and automatically install the version mensioned in packageManager. These installation may also need a proxy or mirrors of npm registry in a limited network environment. I guess author of this PR exprience such cases(as he added proxy config for apt-get as well), and mistakely think it's caused by pnpm.
  4. https://registry.npmjs.org/ is the offical registry, targetting it don't change anything. I change the registry to npmmirror maintaing by Alibaba(there is a benchmark in 2023).

I delete the commented proxy config as well, for users can add their own as needed

Remaining Work: Execution of pip and pnpm using a mirror, but apt-get don't. I wonder in which environment does this Dockfile supposed to be build, or people just adjust it themselves?

Updated 2025.1.3: rebase upon master; add mirror config for apt-get

Copy link

vercel bot commented Jan 2, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
running-page ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 3, 2025 11:38am

@yihong0618
Copy link
Owner

@OrkWard Hi Thank you very much, I merged #761 first can you help to fix the conflicts?

Since package.json already has the packageManager field specified
with pnpm and its specific version, there is no reason not to use
it during whole dev/build process.
@OrkWard
Copy link
Contributor Author

OrkWard commented Jan 3, 2025

Updated, see above

@yihong0618
Copy link
Owner

Updated, see above

do we need to delete the lock file?

@OrkWard
Copy link
Contributor Author

OrkWard commented Jan 3, 2025

do we need to delete the lock file?

Which lock file do you refer to?

To provide more context for why I delete the yarn.lock, just pick a random dependency, say @mapbox/polyline, is resolved to 1.2.1 in yarn.lock and 1.2.0 in pnpm-lock, which probably won't introduce any trouble at present, but is unpredictable in the future as two lock files diverge more and more.

If you refering to the remaining pnpm-lock, it's the core of version management in nodejs environment and should always be included.

@yihong0618 yihong0618 merged commit 4b42c49 into yihong0618:master Jan 4, 2025
4 checks passed
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