Add npm-publish workflow for Node.js package#85
Conversation
This workflow runs tests and publishes a Node.js package to GitHub Packages upon release creation.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughA new GitHub Actions workflow was added to automate Node.js package building, testing, and npm publishing on release creation. The workflow runs a build job that executes npm ci and npm test, followed by a publish-npm job that publishes the package to npm using a stored token. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR adds a new GitHub Actions workflow (npm-publish.yml) that is triggered on release creation and is intended to run tests and publish the package. However, the workflow has several critical issues including a mismatch between the PR description (which mentions GitHub Packages) and the actual registry configuration (npmjs.org), a missing build step that would cause publishing to fail, and redundancy with the existing semantic-release automation already in place.
Changes:
- Added npm-publish.yml workflow with build and publish-npm jobs triggered on release creation
- Workflow runs tests in build job, then publishes to npm registry in publish-npm job
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: 20 |
There was a problem hiding this comment.
The CI workflow includes "cache: 'npm'" in the setup-node action (.github/workflows/ci.yml:21) to speed up dependency installation. This workflow should include the same caching configuration for consistency and improved performance.
| publish-npm: | ||
| needs: build | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
The release-on-merge workflow includes specific permissions configuration for npm publishing (.github/workflows/release-on-merge.yml:16-20), including "id-token: write" for npm provenance. This workflow should include similar permissions, particularly "id-token: write" if publishing with provenance is desired, or at minimum document why different permissions are appropriate.
| - uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: 20 | ||
| registry-url: https://registry.npmjs.org/ |
There was a problem hiding this comment.
The PR description states this workflow publishes to "GitHub Packages", but the registry URL points to npmjs.org (the public npm registry). If the intent is to publish to GitHub Packages, the registry URL should be "https://npm.pkg.github.com/" instead. If publishing to the public npm registry is intended, the PR description should be updated to reflect this.
| - uses: actions/checkout@v4 | ||
| - uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: 20 |
There was a problem hiding this comment.
This workflow specifies Node.js version 20, but the existing CI workflow (.github/workflows/ci.yml:20) and release-on-merge workflow (.github/workflows/release-on-merge.yml:31) both use Node.js version 22. For consistency and to ensure the build environment matches other workflows, this should be updated to version 22.
| - uses: actions/checkout@v4 | ||
| - uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: 20 |
There was a problem hiding this comment.
This workflow specifies Node.js version 20, but the existing CI workflow (.github/workflows/ci.yml:20) and release-on-merge workflow (.github/workflows/release-on-merge.yml:31) both use Node.js version 22. For consistency and to ensure the build environment matches other workflows, this should be updated to version 22.
| with: | ||
| node-version: 20 | ||
| registry-url: https://registry.npmjs.org/ | ||
| - run: npm ci |
There was a problem hiding this comment.
The publish job is missing a build step before running npm publish. The package.json specifies "files": ["dist"], meaning only the dist directory is published. Without running "npm run build" first (as done in .github/workflows/ci.yml:26-27 and .github/workflows/release-on-merge.yml:37-38), the dist directory will not exist or will be out of date, causing the publish to fail or publish stale code.
| - run: npm ci | |
| - run: npm ci | |
| - run: npm run build |
| # This workflow will run tests using node and then publish a package to GitHub Packages when a release is created | ||
| # For more information see: https://docs.github.com/en/actions/publishing-packages/publishing-nodejs-packages | ||
|
|
||
| name: Node.js Package | ||
|
|
||
| on: | ||
| release: | ||
| types: [created] | ||
|
|
||
| jobs: | ||
| build: | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: 20 | ||
| - run: npm ci | ||
| - run: npm test | ||
|
|
||
| publish-npm: | ||
| needs: build | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: 20 | ||
| registry-url: https://registry.npmjs.org/ | ||
| - run: npm ci | ||
| - run: npm publish | ||
| env: | ||
| NODE_AUTH_TOKEN: ${{secrets.npm_token}} |
There was a problem hiding this comment.
The existing release-on-merge.yml workflow already handles automated npm publishing via semantic-release on every push to main (.github/workflows/release-on-merge.yml:43-48). This creates a duplicate publishing mechanism that could cause conflicts. Consider whether this workflow is necessary, or if it serves a different purpose than the existing automated release process. If both are needed, clarify the distinct use cases in comments.
| - uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: 20 |
There was a problem hiding this comment.
The CI workflow includes "cache: 'npm'" in the setup-node action (.github/workflows/ci.yml:21) to speed up dependency installation. This workflow should include the same caching configuration for consistency and improved performance.
This workflow runs tests and publishes a Node.js package to GitHub Packages upon release creation.
Summary by CodeRabbit