-
Notifications
You must be signed in to change notification settings - Fork 557
Update to pnpm 10 #25571
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
Update to pnpm 10 #25571
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.
Pull Request Overview
Updates the project from pnpm 9 to pnpm 10 across all package.json files, including updating package manager specifications and related configuration changes.
- Updates all
packageManagerfields from various pnpm 9.x versions to pnpm 10.17.1 - Updates the main package.json engines field to require pnpm 10
- Updates @types/node dependencies and adds overrides for consistency
Reviewed Changes
Copilot reviewed 21 out of 28 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| package.json | Updates main packageManager to pnpm 10.17.1 and engines requirement to pnpm 10 |
| tools/test-tools/package.json | Updates packageManager field to pnpm 10.17.1 |
| tools/getkeys/package.json | Updates packageManager field to pnpm 10.17.1 |
| tools/benchmark/package.json | Updates packageManager field to pnpm 10.17.1 |
| tools/api-markdown-documenter/package.json | Updates packageManager field to pnpm 10.17.1 |
| server/routerlicious/package.json | Updates packageManager field to pnpm 10.17.1 |
| server/historian/package.json | Updates packageManager field to pnpm 10.17.1 |
| server/gitrest/package.json | Updates packageManager field to pnpm 10.17.1 |
| docs/package.json | Updates packageManager field to pnpm 10.17.1 |
| common/lib/protocol-definitions/package.json | Updates packageManager field to pnpm 10.17.1 |
| common/lib/common-utils/package.json | Updates packageManager and adds pnpm build configuration |
| common/build/eslint-plugin-fluid/package.json | Updates packageManager field to pnpm 10.17.1 |
| common/build/eslint-config-fluid/package.json | Updates packageManager field to pnpm 10.17.1 |
| common/build/build-common/package.json | Updates packageManager field to pnpm 10.17.1 |
| build-tools/packages/version-tools/package.json | Updates @types/node dependency version |
| build-tools/packages/bundle-size-tools/package.json | Updates @types/node dependency version |
| build-tools/packages/build-tools/package.json | Updates @types/node dependency version |
| build-tools/packages/build-infrastructure/src/test/data/testRepo/package.json | Adds packageManager field |
| build-tools/packages/build-infrastructure/package.json | Updates @types/node dependency version |
| build-tools/packages/build-cli/package.json | Updates @types/node dependency version |
| build-tools/package.json | Updates packageManager and adds @types/node override configuration |
Files not reviewed (7)
- build-tools/packages/build-infrastructure/src/test/data/testRepo/pnpm-lock.yaml: Language not supported
- build-tools/pnpm-lock.yaml: Language not supported
- common/lib/common-utils/pnpm-lock.yaml: Language not supported
- common/lib/protocol-definitions/pnpm-lock.yaml: Language not supported
- pnpm-lock.yaml: Language not supported
- server/routerlicious/pnpm-lock.yaml: Language not supported
- tools/benchmark/pnpm-lock.yaml: Language not supported
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.
Guessing this could be reverted and pnpm i would still be happy. OKAY to keep update.
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.
It looks like when I did my repo wide pnpm dedupe I fell victim to pnpm/pnpm#8155 and the less common outcome of dedupe ended up in main. I think keeping this update is the right call.
I have confirmed dedupe is still nondeterministic on this part of this file on pnpm 10.17.1.
| @@ -1,4 +1,4 @@ | |||
| lockfileVersion: '6.0' | |||
| lockfileVersion: '9.0' | |||
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.
fun. Thanks for getting this corrected.
|
I have confirmed that the component detection seems to be working. The lock file format changes (which just change what hashes are used in generated identifiers) do not seem to have broken it. |
| "typescript": "~5.1.6" | ||
| }, | ||
| "packageManager": "pnpm@9.15.7+sha512.ed98f9c748442673c46964b70345bd2282c9b305e8eae539b34ab31d6ef24ef8dd59d8b55f27466f705500b009d9c113471cf87e544f3d5036b297330c26e996", | ||
| "packageManager": "pnpm@10.17.1+sha512.17c560fca4867ae9473a3899ad84a88334914f379be46d455cbf92e5cf4b39d34985d452d2583baf19967fa76cb5c17bc9e245529d0b98745721aa7200ecaf7a", |
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.
Installation scripts for packages are not longer automatically run by default:
Then this one for routerlicious gives me a bit of pause because of this dependency on zookeeper, which apparently does have an install script because
this library will build a native Node.js addon that is fit for the current machine, from the official Apache Zookeeper source code.
The Apache source code is stored in a zip file in the deps folder of this package. Unzipping is made with the decompress package.
Only when you run npm install from a OSX or Windows machine, this step is bypassed because there are already prebuilt binaries for those operating systems.
The script apparently was indeed prevented from running during the server - routerlicious pipeline run... and I'm a bit surprised to not see further build errors, but I do wonder if something is left in a bad state for runtime.
For reference, it does run today (example run).
(I happen to know about this because the fact that this causes a transitive production dep on decompress is getting flagged by component governance.)
All that said... I think this would only affect the docker images we produce for the server components, which a) we don't actually publish for public consumption (because that workflow is broken), so I don't think it would affect consumers, but b) we have plans to start leveraging to run e2e tests against a docker environment instead of the routerlicious environment we currently run on AKS. I'd say let's move forward with this because it's worth it, but it might result in something to figure out for the tests-against-Docker workstream (FYI @Abe27342 , @scarlettjlee , @tylerbutler ).
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.
If we have already installed those packages on that CI image (for example as global installs) then the script is likely a no op. If that is the case, we might need to allow list the script when upgrading the dependency if not upgrading the ci image.
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.
It's not a static image we use for CI, it's about the images we build for the Fluid server components (new images for each commit). Installing the dependencies for the Fluid server packages when building the image is what triggers the install script for the zookeper package.
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.
For reference, this indeed seems to have affected the server images which now go into a crash loop (in a staging environment that doesn't affect our e2e tests against routerlicious; that's the only place where they are deployed automatically) failing with "Invalid node-rdkafka package":
> kubectl logs wiggly-wombat-alfred-57894c99d7-j4fdh
2025-09-30T22:10:28.896Z fluid:services-client Package: @fluidframework/server-services-client - Version: 8.0.0-358343
2025-09-30T22:10:28.932Z fluid:core Package: @fluidframework/server-services-core - Version: 8.0.0-358343
2025-09-30T22:10:29.356Z fluid:memory-orderer Package: @fluidframework/server-memory-orderer - Version: 8.0.0-358343
{"label":"winston","level":"error","message":"alfred service exiting due to error"}
{"errorLabel":"resourceFactory:create","label":"winston","level":"error","message":"Invalid node-rdkafka package","name":"Error","stack":"Error: Invalid node-rdkafka package\n at new RdkafkaBase (/usr/src/server/packages/services-ordering-rdkafka/dist/rdkafkaBase.js:20:19)\n at new RdkafkaProducer (/usr/src/server/packages/services-ordering-rdkafka/dist/rdkafkaProducer.js:18:9)\n at Object.createProducer (/usr/src/server/packages/services/dist/kafkaProducerFactory.js:25:20)\n at AlfredResourcesFactory.create (/usr/src/server/packages/routerlicious-base/dist/alfred/runnerFactory.js:124:33)\n at run (/usr/src/server/packages/services-shared/dist/runner.js:26:45)\n at process.processTicksAndRejections (node:internal/process/task_queues:95:5)"}
{"durationInMs":7.056483999999955,"eventName":"RunService","exception":{"errorLabel":"resourceFactory:create","message":"Invalid node-rdkafka package","name":"Error","stack":"Error: Invalid node-rdkafka package\n at new RdkafkaBase (/usr/src/server/packages/services-ordering-rdkafka/dist/rdkafkaBase.js:20:19)\n at new RdkafkaProducer (/usr/src/server/packages/services-ordering-rdkafka/dist/rdkafkaProducer.js:18:9)\n at Object.createProducer (/usr/src/server/packages/services/dist/kafkaProducerFactory.js:25:20)\n at AlfredResourcesFactory.create (/usr/src/server/packages/routerlicious-base/dist/alfred/runnerFactory.js:124:33)\n at run (/usr/src/server/packages/services-shared/dist/runner.js:26:45)\n at process.processTicksAndRejections (node:internal/process/task_queues:95:5)"},"id":"86755116-7e29-40cc-a500-6516747953bc","label":"winston","level":"error","message":"alfred service exiting due to error","properties":"{\"errorLabel\":\"resourceFactory:create\"}","successful":false,"timestamp":"2025-09-30T22:10:29.407Z","type":"Metric"}node-rdkafka has a script "install": "node-gyp rebuild".
It affected all the components that leverage the routerlicious image (vs the historian or gitrest ones):
Before we move ahead with decommissioning the routerlicious environment in favor of a Docker one spun up on the fly we'll have to address this. FYI @tylerbutler .
|
🔗 Found some broken links! 💔 Run a link check locally to find them. See linkcheck output |
Description
Use pnpm 10.
The lock file format seems unchanged except for using larger hashes in its internal identifiers, which should not cause issues for any tooling.
Breaking Changes
Developers using corepack will be asked to download pnpm if not already cached.
Developers using pnpm some other way may have to install it.
Installation scripts for packages are not longer automatically run by default: only one was found to be needed for CI (puppeteer for common-utils) but it is possible someone will have a use case which needs one when reinstalling a package and will have to approve running a script.
Reviewer Guidance
The review process is outlined on this wiki page.