Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR migrates the project from Yarn v1 to pnpm, updating package manager references throughout the codebase, CI/CD workflows, and documentation.
Changes:
- Replaced Yarn with pnpm in all npm scripts, GitHub Actions workflows, and pre-commit hooks
- Added Node.js ≥18.12 and pnpm ≥9.0 engine requirements
- Updated karma.conf.js with explicit plugins array for pnpm's strict module resolution
- Consolidated and updated development documentation in README.md
- Added OIDC permissions and provenance flag to npm publish workflow
- Removed deprecated CLA signing reference from CONTRIBUTING.md
Reviewed changes
Copilot reviewed 7 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Updated all scripts from yarn to pnpm, added engines field, modified lint-staged config and prepare script |
| karma.conf.js | Added explicit plugins array for pnpm compatibility |
| README.md | Rewrote development setup section with pnpm-specific instructions |
| CONTRIBUTING.md | Removed deprecated CLA signing link |
| .husky/pre-commit | Created husky pre-commit hook using pnpm lint-staged |
| .github/workflows/tests.yml | Added pnpm setup step and updated all commands to use pnpm |
| .github/workflows/npm-publish.yml | Added pnpm setup, OIDC permissions, and simplified publish process with provenance |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
package.json
Outdated
| "test-generate": "bundle exec xdrgen -o generated -n test -l javascript examples/test.x", | ||
| "fmt": "prettier --write '**/*.js'", | ||
| "prepare": "yarn build", | ||
| "prepare": "husky install && pnpm build", |
There was a problem hiding this comment.
The prepare script includes "husky install" which may cause issues when users install this package from npm. The prepare script runs during package installation, but husky is a devDependency and won't be available to end users. While modern husky versions handle this gracefully, it's better practice to conditionally run husky install only in development environments. Consider wrapping it in a try-catch or checking for the presence of husky before running, or use a postinstall script that checks if devDependencies exist.
| "prepare": "husky install && pnpm build", | |
| "prepare": "husky install || true && pnpm build", |
There was a problem hiding this comment.
Agreed and removed
README.md
Outdated
|
|
||
| 1. Clone the repo | ||
| **Requirements:** | ||
| - Node.js ≥ 18.12 |
package.json
Outdated
| "version": "3.1.2", | ||
| "description": "Read/write XDR encoded data structures (RFC 4506)", | ||
| "engines": { | ||
| "node": ">=18.12.0", |
There was a problem hiding this comment.
ah I guess we're not but we should be
There was a problem hiding this comment.
Yea I was on the fence on whether to upgrade in this pr. Its been updated to 20 now. And arguably should be 22 but that can happen when they EOL node 20 in April
There was a problem hiding this comment.
yeah we'll have to do that comprehensively
package.json
Outdated
| "test-generate": "bundle exec xdrgen -o generated -n test -l javascript examples/test.x", | ||
| "fmt": "prettier --write '**/*.js'", | ||
| "prepare": "yarn build", | ||
| "prepare": "husky install && pnpm build", |
Shaptic
left a comment
There was a problem hiding this comment.
I don't see the bump to Node 20 yet but I'll 👍 so you don't have to wait on me once you push that
Summary
This PR migrates the project's package manager from Yarn v1 (Classic) to pnpm
Changes
Package Manager Migration:
CI/CD Updates:
Compatibility Fixes:
Documentation: