feat: Add javascript workflow #3
Conversation
Code Review Summary✨ This pull request introduces robust and well-designed GitHub Actions for JavaScript CI and Release workflows. The implementation demonstrates a strong understanding of GitHub Actions best practices, including composite actions, input validation, and clear status reporting. The comprehensive 🚀 Key Improvements
🚨 Critical Issues
|
![sourceant[bot]](https://avatars.githubusercontent.com/in/1161781?s=60&v=4){kind=small}
| echo "❌ package.json not found in $PWD" | ||
| exit 1 | ||
| fi | ||
|
|
There was a problem hiding this comment.
The check for package.json existence is redundant here. It's already performed in the 'Install dependencies' step (lines 43-52), which will cause the workflow to exit if the file is not found. Removing this redundant check will make the code cleaner and avoid unnecessary duplication.
| echo "🔡 Running npm run typecheck..." | ||
| npm run typecheck |
There was a problem hiding this comment.
Similar to the 'typecheck' script, the README.md states the 'build' script is 'mandatory' (line 242). If run_build is true and the script is missing, the action should exit with an error to ensure this mandatory step is executed or properly configured as a no-op, maintaining consistency with the documented requirements.
| echo "🔡 Running npm run typecheck..." | |
| npm run typecheck | |
| echo "❌ No \"build\" script defined in package.json, but run_build is true. Please define a 'build' script or set 'run_build: false'." | |
| exit 1 |
|
@Lantum-Brendan have you tested this script on an actual project to be sure it's working as expected? |
README.md
Outdated
| │ └── release/ # Create PHP package release | ||
| ├── javascript/ | ||
| │ ├── ci/ # Lint, test, typecheck, build for Node.js/npm libs | ||
| │ └── release/ # Conventional-commit-driven npm release |
There was a problem hiding this comment.
Conventional commit in release?
By the conventional commits are not language specific so if we have to add it somewhere, it should be not be in any language folder
| echo "📦 Installing dependencies before publish..." | ||
| if [ -f "package-lock.json" ]; then | ||
| npm ci | ||
| else | ||
| npm install | ||
| fi |
There was a problem hiding this comment.
For a release workflow, it's critical to ensure reproducible builds. Relying on npm install if package-lock.json is missing can lead to non-deterministic dependency installations. It's best practice to always enforce npm ci for release actions, failing if package-lock.json is not present, as its absence usually indicates an incomplete or improperly committed repository state. This enhances the reliability and predictability of releases.
| echo "📦 Installing dependencies before publish..." | |
| if [ -f "package-lock.json" ]; then | |
| npm ci | |
| else | |
| npm install | |
| fi | |
| if [ ! -f "package-lock.json" ]; then | |
| echo "❌ package-lock.json not found in $PWD" | |
| echo " A 'package-lock.json' file is required for reproducible builds during release." | |
| exit 1 | |
| fi | |
| echo "📦 Using npm ci" | |
| npm ci |
|
Summary of changes based on reviews
2 addresses >> #3 (comment)
|
Yes i copied and tested the flow on a local repo and it worked. |
Added javascript workflow