-
Notifications
You must be signed in to change notification settings - Fork 13
chore: migrate from pnpm to bun package manager #422
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
Conversation
- Replace pnpm with Bun 1.3.3 as package manager - Add bunfig.toml with workspace configuration - Update CI workflow to use oven-sh/setup-bun@v2 - Update lefthook to use bun run - Add workspaces field to package.json for turbo compatibility - Remove pnpm-lock.yaml and pnpm-workspace.yaml
|
📋 PR Review: Package Manager Migration (pnpm → Bun)✅ Positive Changes
|
📋 PR Review: Migrate from pnpm to Bun package manager✅ Overall AssessmentThis is a well-executed migration from pnpm to Bun 1.3.3. The changes are comprehensive, properly configured, and all CI checks are passing. The migration follows best practices for package manager transitions in a monorepo environment. 🔍 Code Quality & Best PracticesExcellent aspects:
Configuration review:
🐛 Potential Issues & RisksLow-risk concerns:
Missing pnpm configuration:
🚀 Performance ConsiderationsPositive impacts:
Considerations:
🔒 Security ConsiderationsPositive aspects:
🧪 Test CoverageExcellent test coverage:
Recommended additional testing:
📝 Documentation Updates RequiredCritical:
Suggested changes to CLAUDE.md:
🎯 RecommendationsBefore merge:
After merge:
✨ ConclusionThis is a high-quality migration that successfully transitions the codebase from pnpm to Bun while maintaining all existing functionality. The technical implementation is sound, CI is passing, and the configuration is properly set up. With the documentation updates completed, this will be a positive change that should improve developer experience through faster dependency management. Approval Status: ✅ APPROVED (pending documentation updates) The migration is technically complete and ready for production use. The documentation updates are recommended but not blocking for the merge. |
- Replace type assertions with proper type guards (hasWeaverseStudio) - Fix encapsulation violation in WeaverseHydrogenItem constructor - Replace 'any' types with specific ComponentDataValue union types - Add memory leak protection to ThemeSettingsStore (max 100 listeners) - Implement proper cleanup with destroy() method - Add error handling for listener callbacks - Make Window interface properties optional for better null safety - Fix error component prop types (Error | unknown instead of any) All TypeScript type checking and builds passing.
Pull Request Review: Package Manager Migration from pnpm to Bun🎯 SummaryThis PR migrates the Weaverse SDK monorepo from pnpm to Bun 1.3.3, which promises better performance and simplified dependency management. The migration is comprehensive and covers build systems, CI/CD workflows, and development tooling. ✅ Strengths1. Comprehensive Migration
2. Configuration Quality
3. Performance Considerations
|
- Downgrade @weaverse/core, @weaverse/hydrogen, and @weaverse/react to version 5.8.4 - Update peer dependencies for react-router to version 7.9.2 - Remove unused package directory from clean script - Update lefthook scripts for compatibility with local installation paths - Update submodule reference in templates/pilot
📋 Code Review SummaryI have reviewed PR 422 which migrates the Weaverse SDKs monorepo from pnpm to Bun 1.3.3. ✅ Areas Done Well1. Complete Migration Coverage
2. Configuration Quality
3. CI/CD Improvements
|
- Remove hardcoded absolute paths from lefthook hooks (pre-commit, prepare-commit-msg) - Update Node.js engine requirement from >=22 to >=18 - Update CLAUDE.md documentation to replace pnpm references with bun
📋 PR Review: Migrate from pnpm to BunOverall AssessmentThis is a well-executed migration from pnpm to Bun with significant additional improvements to type safety and code quality. The changes are comprehensive and follow the repository's established patterns. ✅ Positive ChangesPackage Manager Migration
Type Safety Improvements
Code Quality Enhancements
🔍 Areas of ConsiderationMinor Issues
Testing Recommendations
🚀 Performance & SecurityPerformance
Security
📝 Documentation & MaintainabilityDocumentation Quality
Maintainability
🔄 Migration StrategyThe migration approach is excellent:
🎯 RecommendationsImmediate Actions
Future Considerations
SummaryThis is an excellent PR that combines a necessary package manager migration with significant code quality improvements. The type safety enhancements, memory management improvements, and proper error handling make this more than just a migration - it's a substantial upgrade to the codebase quality. Rating: 🟢 Approve with Confidence The changes are well-thought-out, follow best practices, and improve the overall code quality while successfully migrating to Bun. |
- Reverted complex prepareInitialData logic that broke component registration - Restored simple constructor that works with existing pilot template - Kept important fixes: hasWeaverseStudio type guard and error handling improvements - Pilot template now works correctly again
🔍 PR Review: Package Manager Migration (pnpm → Bun)✅ Overall AssessmentThis is a well-executed migration from pnpm to Bun 1.3.3. The changes are comprehensive and follow best practices for package manager migration in a monorepo setup. 🎯 Key Changes Reviewed1. Configuration Updates
2. CI/CD Pipeline (.github/workflows/check.yml)
3. Documentation Updates (CLAUDE.md)
4. File Cleanup
|
- Document critical design principle about constructor timing and deferred processing - Add historical context about commit 495e122 that broke pilot template - Fix defaultComponents to properly handle default exports - This serves as a guide for future developers to avoid similar timing issues
📋 PR Review: chore: migrate from pnpm to bun package manager🎯 Overall AssessmentThis is a well-executed migration from pnpm to Bun 1.3.3 that modernizes the package management while maintaining backward compatibility. The changes are comprehensive and follow best practices. All CI checks are passing, which is excellent. ✅ Strengths & Positive PointsConfiguration Changes
Documentation Updates
Clean Migration
🚨 Areas of Concern & Recommendations1. Version Pinning & Compatibility
2. Node.js Engine Requirements
3. Missing Migration Steps
🔍 Code Quality AnalysisGitHub Actions
Package.json Changes
🛡️ Security Considerations
📊 Performance ImpactExpected Benefits:
🎯 Testing Coverage
📝 Final Recommendations
🚀 ConclusionThis is an excellent migration that modernizes the project's tooling while maintaining stability. The changes are comprehensive, well-tested, and follow best practices. The migration brings performance benefits while preserving existing workflows. Recommendation: ✅ Approve and Merge - with minor suggestions for version pinning flexibility. |
- Added support for local lefthook binary in pre-commit and prepare-commit-msg scripts - Updated submodule reference in templates/pilot to indicate a dirty state
📋 Pull Request Review✅ Overall AssessmentThis is a well-executed migration from pnpm to Bun 1.3.3 with significant TypeScript improvements. The changes align perfectly with the project's goals and maintain backward compatibility. 🔧 Package Manager MigrationStrengths:
Notes:
🏷️ TypeScript ImprovementsExcellent enhancements:
Specific improvements:
🏗️ Code QualityArchitecture:
Documentation:
⚡ Performance & SecurityPerformance:
Security:
🧪 Testing ConsiderationsTest Plan Coverage:
Additional Testing Recommendations:
📝 Minor Suggestions
🎯 SummaryThis PR successfully:
The changes are production-ready and represent a net positive for the codebase. The TypeScript improvements alone make this valuable, even before considering the package manager migration benefits. Recommendation: ✅ Approve and Merge |
Summary
workspacesfield and bunfig.tomloven-sh/setup-bun@v2Test plan
bun run buildpasses (all packages)bun run typecheckpassesbun run biomepasses