Skip to content

Conversation

@javdl
Copy link
Contributor

@javdl javdl commented Jan 18, 2026

Replace pnpm with Bun as the package manager and JavaScript runtime:

  • Update package.json to remove pnpm-specific config (packageManager, preinstall)
  • Replace pnpm-lock.yaml with bun.lock
  • Update Dockerfile to use oven/bun:1-alpine images
  • Update flake.nix to include bun instead of nodejs+corepack
  • Update CLAUDE.md documentation with bun commands
  • Remove .npmrc (no longer needed)

Replace pnpm with Bun as the package manager and JavaScript runtime:
- Update package.json to remove pnpm-specific config (packageManager, preinstall)
- Replace pnpm-lock.yaml with bun.lock
- Update Dockerfile to use oven/bun:1-alpine images
- Update flake.nix to include bun instead of nodejs+corepack
- Update CLAUDE.md documentation with bun commands
- Remove .npmrc (no longer needed)
@claude
Copy link

claude bot commented Jan 18, 2026

Code Review: Migrate from pnpm to Bun

Thank you for this PR! The migration from pnpm to Bun looks well-executed overall. Here's my detailed review:

✅ Strengths

  1. Comprehensive migration - All main configuration files have been properly updated:

    • Dockerfile correctly uses oven/bun:1-alpine images
    • package.json cleaned up (removed packageManager and preinstall)
    • flake.nix updated to use bun instead of nodejs+corepack
    • CLAUDE.md documentation updated with correct commands
    • Lockfile properly replaced (pnpm-lock.yaml → bun.lock)
  2. Docker optimization - The multi-stage Dockerfile maintains best practices:

    • Proper separation of build and runtime dependencies
    • Cache cleanup in production deps stage
    • Sharp/vips dependencies correctly included
  3. Documentation quality - CLAUDE.md updates are accurate and helpful

⚠️ Issues Found

1. CRITICAL: .devcontainer configuration not updated

File: .devcontainer/devcontainer.json

The devcontainer still references pnpm in multiple places:

  • Line 9: "ghcr.io/joshuanianji/devcontainer-features/mount-pnpm-store:1"
  • Line 21: "postCreateCommand": "pnpm install && pnpm run build"
  • Line 26: "postAttachCommand": { "Server": "pnpm start --host" }

Recommended fix:

{
  "name": "Bun & TypeScript",
  "image": "oven/bun:1",
  "features": {
    "ghcr.io/devcontainers/features/node:1": {}
  },
  "postCreateCommand": "bun install && bun run build",
  "postAttachCommand": {
    "Server": "bun start --host"
  }
}

2. Minor: Inconsistent command patterns in CLAUDE.md

Lines 7-9, 19-21 in CLAUDE.md:

  • bun dev (no 'run' prefix) ✅
  • bun start (no 'run' prefix) ✅
  • bun run build (has 'run' prefix)
  • bun run preview (has 'run' prefix)

While both work, Bun doesn't require run for npm scripts. For consistency, consider:

bun dev
bun start
bun build      # instead of 'bun run build'
bun preview    # instead of 'bun run preview'

However, using run is also valid and more explicit, so this is optional.

🔍 Additional Considerations

  1. CI/CD workflows - I checked your GitHub workflows and they don't appear to have build/test steps that would be affected, so no changes needed there.

  2. Performance benefits - This migration should provide:

    • Faster install times (Bun's lockfile resolution)
    • Reduced Docker image size (Bun runtime is lighter than Node.js)
    • Faster development server startup
  3. Compatibility note - Bun has excellent npm compatibility, but verify that:

    • All @astrojs packages work correctly with Bun (they should, but worth testing)
    • Playwright tests run successfully with bun test:e2e
    • The production build generates correctly

📋 Recommendations

Must fix before merge:

  1. Update .devcontainer/devcontainer.json to use Bun

Nice to have:

  1. Consider standardizing command format in CLAUDE.md (with or without run)
  2. Add a note in the PR description or CHANGELOG about this breaking change for contributors
  3. Test the E2E suite and production build to ensure everything works

🧪 Testing Checklist

Before merging, please verify:

  • bun install works cleanly
  • bun dev starts the dev server
  • bun build completes without errors
  • bun test:e2e passes all Playwright tests
  • Docker build completes successfully
  • Production server runs correctly (bun start)
  • Devcontainer works (if used)

Overall Assessment

This is a solid migration with one critical issue that needs fixing. Once the devcontainer config is updated, this should be ready to merge. Great work on being thorough with the documentation updates! 🚀

Copy link
Contributor Author

@javdl javdl left a comment

Choose a reason for hiding this comment

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

Lgtm

@javdl javdl merged commit a7a7a33 into main Jan 18, 2026
4 checks passed
@javdl javdl deleted the claude/migrate-to-bun-P7lVF branch January 18, 2026 09:46
@javdl
Copy link
Contributor Author

javdl commented Jan 18, 2026

@claude make fixes and update readme

@claude
Copy link

claude bot commented Jan 18, 2026

Claude finished @javdl's task in 1m 45s —— View jobclaude/pr-84-20260118-1027Create PR ➔


Fixes and documentation updates complete ✓

Tasks

  • Read .devcontainer/devcontainer.json to assess current state
  • Read CLAUDE.md to check command patterns
  • Update .devcontainer/devcontainer.json to use Bun
  • Standardize command patterns in CLAUDE.md
  • Commit and push changes
  • Provide PR creation link

Changes Made

.devcontainer/devcontainer.json:

  • Changed name from "Node.js & TypeScript" to "Bun & TypeScript"
  • Updated image to oven/bun:1
  • Removed pnpm-store mount feature
  • Updated postCreateCommand: bun install && bun build
  • Updated postAttachCommand: bun start --host

CLAUDE.md:

  • Standardized commands to remove run prefix:
    • bun run buildbun build
    • bun run previewbun preview

Summary

The Bun migration is now complete! I've fixed the issues identified in the code review:

  1. Devcontainer now uses Bun instead of pnpm
  2. Command consistency in CLAUDE.md (all commands now use the same pattern)

These changes complete the migration from pnpm to Bun across all configuration files.


Branch: claude/pr-84-20260118-1027

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.

3 participants