feat: add publish to npm workflow - #164
Conversation
Control PanelHi, I'm an AI-powered Review Bot that helps you with summarizing and reviewing pull requests.
|
![hyperspace-pr-bot[bot]](https://avatars.githubusercontent.com/u/2531208?s=60&v=4){kind=small}
There was a problem hiding this comment.
The PR introduces a well-structured refactoring of the CI/CD pipeline—splitting the monolithic build-and-publish workflow into composable reusable workflows—but there are several correctness issues: missing artifact path in the publish step, an unquoted glob passed directly to npm publish, a script-injection risk with the dist-tag expression, dropped --ignore-scripts flags that were intentional in the original Expo sample install steps, and an unnecessary registry-url in the build/pack job. Please address the flagged items before merging.
PR Bot Information
Version: 1.31.20
- Correlation ID:
05089b00-acee-11f1-898c-20785066999e - File Content Strategy: Full file content
- LLM:
anthropic--claude-4.6-sonnet - Event Trigger:
issue_comment.edited
| uses: actions/setup-node@v5 | ||
| with: | ||
| node-version: '20' | ||
| registry-url: 'https://registry.npmjs.org' |
There was a problem hiding this comment.
Bug: registry-url is unnecessary in the build/pack step and leaks npm registry config into a step that doesn't publish.
registry-url is only needed when actually publishing (it writes the .npmrc with the auth token). It has no effect on npm ci, linting, testing, building, or packing, and could interfere if the registry is unreachable. Move it to publish-npm.yml only, which already has it correctly.
| registry-url: 'https://registry.npmjs.org' |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| - name: Dry run publish | ||
| run: npm publish --dry-run ./artifacts/*.tgz | ||
| env: | ||
| NODE_AUTH_TOKEN: ${{ secrets.NPM_PUBLISHER_TOKEN }} |
There was a problem hiding this comment.
Bug: npm publish --dry-run ./artifacts/*.tgz will fail because npm publish does not accept a tarball path as an argument when a NODE_AUTH_TOKEN is set and the working directory is not the package root. More importantly, npm publish <tarball> ignores --dry-run in older npm versions, and in all versions the glob ./artifacts/*.tgz is shell-expanded before npm sees it — if there are zero or multiple files the command silently does the wrong thing.
Consider using npm publish --dry-run from the pack destination directory, or explicitly quote and validate the single .tgz file path before passing it:
| - name: Dry run publish | |
| run: npm publish --dry-run ./artifacts/*.tgz | |
| env: | |
| NODE_AUTH_TOKEN: ${{ secrets.NPM_PUBLISHER_TOKEN }} | |
| - name: Dry run publish | |
| run: | | |
| TGZ=$(echo artifacts/*.tgz) | |
| npm publish --dry-run "$TGZ" | |
| env: | |
| NODE_AUTH_TOKEN: ${{ secrets.NPM_PUBLISHER_TOKEN }} |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| - name: Download package artifact | ||
| uses: actions/download-artifact@v6 | ||
| with: | ||
| name: npm-package |
There was a problem hiding this comment.
Bug: The download-artifact step does not specify a path, so the .tgz is downloaded to the runner's working directory root. The subsequent tar and npm publish commands use the bare glob *.tgz, which happens to match — but this is fragile. If the working directory ever changes or the artifact contains subdirectories, the glob will silently resolve incorrectly. Specifying an explicit path: artifacts (matching the upload convention in build-package.yml) and adjusting the glob to artifacts/*.tgz makes the intent explicit and consistent.
| - name: Download package artifact | |
| uses: actions/download-artifact@v6 | |
| with: | |
| name: npm-package | |
| - name: Download package artifact | |
| uses: actions/download-artifact@v6 | |
| with: | |
| name: npm-package | |
| path: artifacts |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| - name: Publish to npm | ||
| run: | | ||
| echo "Publishing @emartech/react-native-emarsys-sdk to npm with tag ${{ steps.dist_tag.outputs.tag }}" | ||
| npm publish *.tgz --access public --tag ${{ steps.dist_tag.outputs.tag }} | ||
| env: | ||
| NODE_AUTH_TOKEN: ${{ secrets.NPM_PUBLISHER_TOKEN }} |
There was a problem hiding this comment.
Security / Bug: The dist-tag value from steps.dist_tag.outputs.tag is interpolated directly into the shell command via ${{ }} expression syntax rather than being passed as an environment variable. While TAG is hardcoded to either beta or latest in this workflow, directly embedding workflow expressions into run scripts is a GitHub Actions injection anti-pattern. If the logic ever changes to use version-derived strings, it becomes an injection vector. Should use an env: variable instead:
| - name: Publish to npm | |
| run: | | |
| echo "Publishing @emartech/react-native-emarsys-sdk to npm with tag ${{ steps.dist_tag.outputs.tag }}" | |
| npm publish *.tgz --access public --tag ${{ steps.dist_tag.outputs.tag }} | |
| env: | |
| NODE_AUTH_TOKEN: ${{ secrets.NPM_PUBLISHER_TOKEN }} | |
| - name: Publish to npm | |
| run: | | |
| echo "Publishing @emartech/react-native-emarsys-sdk to npm with tag $DIST_TAG" | |
| npm publish *.tgz --access public --tag "$DIST_TAG" | |
| env: | |
| NODE_AUTH_TOKEN: ${{ secrets.NPM_PUBLISHER_TOKEN }} | |
| DIST_TAG: ${{ steps.dist_tag.outputs.tag }} |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| id: dist_tag | ||
| run: | | ||
| VERSION=$(tar -xzO -f *.tgz package/package.json | node -pe "JSON.parse(require('fs').readFileSync(0)).version") | ||
| if [[ "$VERSION" == *-* ]]; then |
There was a problem hiding this comment.
Logic Error: The pre-release version detection only checks for a hyphen (*-*), which matches the SemVer pre-release convention (e.g. 2.0.0-beta.2). However, version strings like 2.0.0-0 (numeric-only pre-release) or build metadata variants such as 2.0.0+build.1 would be misclassified: the latter would be tagged latest despite not being a stable release. For the current naming scheme this works, but the check is worth tightening to explicitly match SemVer pre-release labels to avoid a mis-tagged publish in edge cases.
| if [[ "$VERSION" == *-* ]]; then | |
| if [[ "$VERSION" =~ ^[0-9]+\.[0-9]+\.[0-9]+-.+ ]]; then |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| npm uninstall @emartech/react-native-emarsys-sdk | ||
|
|
||
| # install from generated artifact | ||
| TGZ_ABS=$(realpath ../../artifacts/*.tgz) | ||
| npm install "$TGZ_ABS" |
There was a problem hiding this comment.
Bug: Compared to the removed build-sample-apps.yml, the npm uninstall call here drops the --ignore-scripts flag that was present in the original. Running postinstall/preuninstall scripts during uninstall could fail or produce side effects in a CI context where the package was only installed as a dev dependency. The install step also drops --ignore-scripts from npm install "$TGZ_ABS". The original was intentional: autolinking must use the tarball but install scripts should not run during the swap. Should restore --ignore-scripts to match the prior behaviour.
| npm uninstall @emartech/react-native-emarsys-sdk | |
| # install from generated artifact | |
| TGZ_ABS=$(realpath ../../artifacts/*.tgz) | |
| npm install "$TGZ_ABS" | |
| npm uninstall @emartech/react-native-emarsys-sdk --ignore-scripts | |
| # install from generated artifact | |
| TGZ_ABS=$(realpath ../../artifacts/*.tgz) | |
| npm install "$TGZ_ABS" --ignore-scripts |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
2 participants
No description provided.