-
Notifications
You must be signed in to change notification settings - Fork 10
Replace Bun with Node.js #13
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 bun with npm/node in package.json scripts - Update RELEASE.md to reference npm commands throughout - Remove bun.lock and add package-lock.json - Remove bun engine requirement from package.json This change improves compatibility with standard Node.js tooling and aligns with the augment-agent repository changes.
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.
Great job on the migration to Node.js; the changes appear consistent and focused. I left a couple notes about a documentation inconsistency referencing a non-existent script.
|
|
||
| ```bash | ||
| bun run typecheck | ||
| npm run typecheck |
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.
This step references npm run typecheck, but package.json does not define a typecheck script, so it will fail. Consider aligning the docs with the available scripts (related note also appears later in this file).
🤖 React with 👍 or 👎 to let us know if the comment was useful.
| **Type check failures:** | ||
|
|
||
| - Run `bun run typecheck` locally to check for TypeScript errors | ||
| - Run `npm run typecheck` locally to check for TypeScript errors |
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.
This also references npm run typecheck, but there is no typecheck script in package.json (related to the earlier comment in this file). You could update the docs or add the missing script to avoid confusion.
🤖 React with 👍 or 👎 to let us know if the comment was useful.
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.
Review Summary
The changes to package.json, RELEASE.md, and lock files look good. However, the Bun-to-Node.js migration is incomplete - there are critical GitHub Actions workflows that still reference Bun and will fail with the current changes.
Critical Issues Found
1. .github/workflows/format-check.yml still uses Bun
This workflow needs to be updated to use Node.js:
Lines 19-28 need updating:
# Current (uses Bun)
- name: Setup Bun
uses: oven-sh/setup-bun@735343b667d3e6f658f44d0eca948eb6282f2b76 # v2.0.2
with:
bun-version: latest
- name: Install dependencies
run: bun install --frozen-lockfile
- name: Check formatting
run: bun run format:checkShould be:
- name: Setup Node.js
uses: actions/setup-node@v4
with:
node-version: '22'
- name: Install dependencies
run: npm ci
- name: Check formatting
run: npm run format:checkLine 33 error message:
# Current
echo "❌ Files are not properly formatted. Run 'bun run format' to fix."
# Should be
echo "❌ Files are not properly formatted. Run 'npm run format' to fix."2. .github/workflows/release.yml still uses Bun
Lines 23-29 need updating:
# Current (uses Bun)
- name: Setup Bun
uses: oven-sh/setup-bun@735343b667d3e6f658f44d0eca948eb6282f2b76 # v2.0.2
with:
bun-version: latest
- name: Install dependencies
run: bun installShould be:
- name: Setup Node.js
uses: actions/setup-node@v4
with:
node-version: '22'
- name: Install dependencies
run: npm ci3. .prettierignore references obsolete Bun lock files
Lines 60-61 should be removed:
bun.lock
bun.lockb
These are no longer relevant since the project now uses package-lock.json.
Recommendation
These workflow files need to be updated in this PR to complete the migration. Without these changes, CI/CD pipelines will fail because they'll try to use Bun commands with a Node.js-only setup.
🤖 Automated review complete. Please react with 👍 or 👎 on the individual review comments to provide feedback on their usefulness.
8cb4c1d to
84262b2
Compare
Replace Bun with Node.js as the runtime for development and testing.
Key Changes
bun.lockwithpackage-lock.jsonand update engine requirements to specify Node.js ≥22.0.0 onlypackage.jsonscripts to usenpm/nodecommands instead ofbunRELEASE.mdto referencenpmcommands throughoutTechnical Impact
This change improves compatibility with standard Node.js tooling and aligns with changes made in the augment-agent repository (see augment-agent PR #9).
Testing