feat: add workspace support - #20
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughChangesRelated PRs: None specified. The reusable publish workflow now accepts 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nodejs-publish/action.yml (1)
61-91: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDetect workspaces from the root package.json
inputs.version_fileis documented to point at a package-level./package.jsonfor workspace projects, sorequire(process.env.VERSION_FILE).workspaceswill stay empty and--workspacesnever gets added. Read the root manifest here (or add a separate root input) in both steps.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nodejs-publish/action.yml` around lines 61 - 91, The workspace detection in the Publish to npm and Pack npm artifact steps is reading inputs.version_file, which points at a package-level manifest and can miss the root workspaces field. Update the logic in both bash blocks to read the root package.json for workspace detection, or introduce a dedicated root manifest input, and keep the existing WORKSPACES_FLAGS and npm publish/npm pack flow unchanged otherwise.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nodejs-publish/action.yml`:
- Around line 16-21: The Get version step in action.yml is hiding failures from
the node -p call because the command substitution is wrapped inside echo, so a
bad VERSION_FILE can silently produce an empty output. Update the version
retrieval logic so the node invocation is executed as a standalone
command/assignment in the Get version step, letting bash errexit fail the step
immediately; keep the id version and GITHUB_OUTPUT write path, but ensure the
version read from process.env.VERSION_FILE cannot be swallowed by echo.
- Around line 116-138: The release asset upload path in the `gh release edit`
branch is not idempotent because `gh release upload` will fail on duplicate
tarball names during reruns. Update the upload loop in `action.yml` to make `gh
release upload` safe for retries, using `--clobber` or equivalent pre हट removal
of existing assets, while preserving the existing `RELEASE_VERSION`,
`TARBALL_ARGS`, and release edit/create flow.
---
Outside diff comments:
In `@nodejs-publish/action.yml`:
- Around line 61-91: The workspace detection in the Publish to npm and Pack npm
artifact steps is reading inputs.version_file, which points at a package-level
manifest and can miss the root workspaces field. Update the logic in both bash
blocks to read the root package.json for workspace detection, or introduce a
dedicated root manifest input, and keep the existing WORKSPACES_FLAGS and npm
publish/npm pack flow unchanged otherwise.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7ceb94a5-29ea-498c-8517-610076ef8a23
📒 Files selected for processing (2)
.github/workflows/nodejs-publish-release.ymlnodejs-publish/action.yml
5b06c2a to
8eb116a
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
nodejs-publish/action.yml (2)
70-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated workspace-detection logic across two steps.
The
WORKSPACES_FLAGSdetection block (node -e "const p = require('./package.json')...") is copy-pasted identically in both "Publish to npm" (lines 70-73) and "Pack npm artifact" (lines 85-88). Any future change to the detection logic (e.g., supporting a different workspaces config shape) risks drifting between the two copies.Consider hoisting this into a single earlier step (e.g., "Detect workspace") that writes a
GITHUB_OUTPUTvalue, then reference it viasteps.<id>.outputs.is_workspacein both downstream steps.♻️ Proposed refactor
+ - name: Detect workspace + id: workspace + shell: bash + run: | + if node -e "const p = require('./package.json'); process.exit(p.workspaces ? 0 : 1)" 2>/dev/null; then + echo "is_workspace=true" >> "$GITHUB_OUTPUT" + else + echo "is_workspace=false" >> "$GITHUB_OUTPUT" + fi + - name: Publish to npm shell: bash env: RELEASE_VERSION: ${{ steps.version.outputs.value }} NODE_AUTH_TOKEN: ${{ inputs.npm_token }} GITHUB_TOKEN: ${{ github.token }} # Required for napi-rs support run: | WORKSPACES_FLAGS=() - if node -e "const p = require('./package.json'); process.exit(p.workspaces ? 0 : 1)" 2>/dev/null; then + if [ "${{ steps.workspace.outputs.is_workspace }}" = "true" ]; then WORKSPACES_FLAGS=("--workspaces") fi ... - name: Pack npm artifact id: pack shell: bash run: | WORKSPACES_FLAGS=() - if node -e "const p = require('./package.json'); process.exit(p.workspaces ? 0 : 1)" 2>/dev/null; then + if [ "${{ steps.workspace.outputs.is_workspace }}" = "true" ]; then WORKSPACES_FLAGS=("--workspaces") fi ...🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nodejs-publish/action.yml` around lines 70 - 90, The workspace-detection logic is duplicated in both the publish and pack steps, so move the `node -e` check into a single earlier step (for example, a dedicated workspace detection step) and write the result to `GITHUB_OUTPUT`. Then update both `Publish to npm` and `Pack npm artifact` to read the shared output instead of rebuilding `WORKSPACES_FLAGS` locally, keeping the existing behavior in sync.
22-23: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
version_filerequires a./-prefixed (or absolute) path to resolve viarequire().
require(process.env.VERSION_FILE)treats strings not starting with./,../, or/as a module-lookup, not a relative file path. The inputdescriptiondocuments the./convention, and a misconfigured value will now fail loudly (good, thanks to the earlier fix) rather than silently — but the failure message (Cannot find module '<value>') may confuse consumers who omit the prefix.Optionally normalize the path defensively, e.g.
path.resolve(process.cwd(), process.env.VERSION_FILE), to accept paths with or without the./prefix.Also applies to: 32-32
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nodejs-publish/action.yml` around lines 22 - 23, The VERSION_FILE lookup in the action’s version-reading step still assumes a require()-resolvable path, so inputs without a ./, ../, or / prefix will be treated as module names and fail confusingly. Update the version extraction logic around the node -p require(process.env.VERSION_FILE) usage to normalize the input path first, such as by resolving it from process.cwd(), so the action accepts version_file values with or without a ./ prefix while keeping the existing output assignment to GITHUB_OUTPUT intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@nodejs-publish/action.yml`:
- Around line 70-90: The workspace-detection logic is duplicated in both the
publish and pack steps, so move the `node -e` check into a single earlier step
(for example, a dedicated workspace detection step) and write the result to
`GITHUB_OUTPUT`. Then update both `Publish to npm` and `Pack npm artifact` to
read the shared output instead of rebuilding `WORKSPACES_FLAGS` locally, keeping
the existing behavior in sync.
- Around line 22-23: The VERSION_FILE lookup in the action’s version-reading
step still assumes a require()-resolvable path, so inputs without a ./, ../, or
/ prefix will be treated as module names and fail confusingly. Update the
version extraction logic around the node -p require(process.env.VERSION_FILE)
usage to normalize the input path first, such as by resolving it from
process.cwd(), so the action accepts version_file values with or without a ./
prefix while keeping the existing output assignment to GITHUB_OUTPUT intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f92c95ed-a07a-4e9f-9387-758091d05cfa
📒 Files selected for processing (2)
.github/workflows/nodejs-publish-release.ymlnodejs-publish/action.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/nodejs-publish-release.yml
8eb116a to
1b72f14
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Around line 91-99: The publish workflow example is missing the newly required
version_file input, so readers can still copy an incomplete configuration and
fail at release time. Update the nodejs-publish-release usage example and the
workspace guidance in the README to explicitly include version_file alongside
workspace_main_package, and make sure the description of required inputs
reflects that the workflow now reads the release version from that file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
caf3239 to
08eca6b
Compare
|
✔️ 0b340f8...08eca6b - Conventional commits check succeeded. |
3 participants
Automatically detect if the project is in a workspace and use the
--workspaceflag fornpm releaseif it is. The newversion_fileinput is required to know where to read the new version from and to get the package name to check if that version already exists.This assumes that all the packages are released under the same version.
This successfully worked with publishing
v0.0.8of the NPM test repo: https://github.com/holochain/npm-release-test/actions/runs/28855732581