-
Notifications
You must be signed in to change notification settings - Fork 11
Remove dependency on bun #8
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
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 PR attempts to remove the Bun dependency, but the implementation has critical issues that will prevent the action from working. The main problems are: incorrect npm command syntax, missing TypeScript execution infrastructure, and incompatible shebang.
🤖 Automated review complete. Please react with 👍 or 👎 on the individual review comments to provide feedback on their usefulness.
action.yml
Outdated
| with: | ||
| bun-version: latest | ||
| - name: Test list directory | ||
| run: ls |
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.
Critical Issue: The command npm run $GITHUB_ACTION_PATH/src/index.ts is incorrect and will fail.
npm run expects a script name defined in package.json, not a file path. For example, npm run dev or npm run test.
Current code:
run: npm run $GITHUB_ACTION_PATH/src/index.tsThe problem: There's no script in package.json that matches this path, and npm run doesn't accept file paths as arguments.
Suggested approach: You need to either:
- Use Node.js directly with a TypeScript runner:
run: node --import tsx $GITHUB_ACTION_PATH/src/index.ts(Requires adding tsx as a dependency)
- Or use Node.js 22's experimental TypeScript support:
run: node --experimental-strip-types $GITHUB_ACTION_PATH/src/index.ts- Or compile TypeScript first, then run the compiled JavaScript:
run: |
cd $GITHUB_ACTION_PATH
npm install
npx tsc
node dist/index.jsNote: Option 2 is the simplest since Node 22 is already being set up, but it's still experimental. Option 1 with tsx is more stable.
action.yml
Outdated
| with: | ||
| bun-version: latest | ||
| - name: Test list directory | ||
| run: ls |
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.
Additional Issue: The action doesn't install dependencies before running the script.
The TypeScript code imports several npm packages (@actions/core, nunjucks, @octokit/rest, etc.) that need to be installed. Without running npm install first, these imports will fail.
Suggested improvement:
- name: Install dependencies
run: |
cd $GITHUB_ACTION_PATH
npm install --production
shell: bash
- name: Run Augment Agent
run: node --experimental-strip-types $GITHUB_ACTION_PATH/src/index.ts
shell: bashThe --production flag ensures only runtime dependencies are installed, not devDependencies.
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.
Thanks for simplifying the action by removing the Bun step. I left one comment about the new run command that likely won’t execute as intended. Otherwise the change looks straightforward.
action.yml
Outdated
| bun-version: latest | ||
| - name: Test list directory | ||
| run: ls | ||
| shell: bash |
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.
Using npm run $GITHUB_ACTION_PATH/src/index.ts is unlikely to work because npm run expects a script name defined in a local package.json (not a file path), so this will likely fail with a missing script. Since the path points under $GITHUB_ACTION_PATH, there may be no matching script in the workflow’s working directory (also applies to other environments using this action).
🤖 React with 👍 or 👎 to let us know if the comment was useful.
Additional Migration ConsiderationsBeyond the issues noted in the inline review comments, there are several other aspects of the codebase that need to be addressed for a complete migration from Bun to Node.js: 1. Shebang in src/index.tsThe file currently has 2. Other Workflows Still Use BunSeveral GitHub Actions workflows in
These workflows will need to be updated to use Node.js and npm instead. 3. Package.json ScriptsAll scripts in package.json use Bun commands: "test": "bun test",
"typecheck": "bun --bun tsc --noEmit",
"dev": "bun --watch src/index.ts"These should be updated to use npm/node equivalents. 4. Documentation UpdatesThe RELEASE.md file references Bun commands (e.g., 5. Lock FileThe project has
Recommendation: This PR should be expanded to include all these changes for a complete migration, or the scope should be clarified if only partial migration is intended. |
Remove Bun runtime dependency from GitHub Action
This PR simplifies the action setup by removing the Bun runtime dependency and switching to npm for execution.
Changes:
Setup Bunstep from the workflowbun runwithnpm runfor executing the action scriptImpact:
Reduces external dependencies and simplifies the action's setup process, making it easier to maintain and potentially faster to initialize.
🤖 This description was generated automatically. Please react with 👍 if it's helpful or 👎 if it needs improvement.