Add Node.js prerequisite check to make deps target#4168
Add Node.js prerequisite check to make deps target#4168
Conversation
- Add Node.js/npm availability check before installing prettier - Provide helpful error message with installation instructions for multiple platforms - Update CONTRIBUTING.md and DEVGUIDE.md to list Node.js as prerequisite - Follows existing pattern from golint target for missing tool detection Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot make sure devcontainer.json has node and nvm installed |
- Add nvmVersion: latest to node feature in devcontainer.json - Ensures nvm is explicitly available in the development container - Node.js 24 was already configured, now nvm is also explicitly set Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
This PR addresses issue #4160 by adding prerequisite checks and documentation to prevent silent failures when Node.js is not installed. The make deps target previously would silently install prettier without Node.js, leading to cryptic "node: not found" errors during subsequent development tasks.
Key changes:
- Added fail-fast Node.js/npm availability check to
make depstarget with helpful installation instructions - Updated prerequisites documentation in CONTRIBUTING.md and DEVGUIDE.md to explicitly require Node.js 24+
- Enhanced devcontainer configuration with explicit nvm support
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| Makefile | Added Node.js prerequisite check before prettier installation with detailed installation instructions for nvm, Ubuntu/Debian, and macOS |
| DEVGUIDE.md | Added comprehensive prerequisites section documenting Node.js 24+ requirement along with Go, GitHub CLI, and Git |
| CONTRIBUTING.md | Added Node.js 24+ to prerequisites list for consistency with DEVGUIDE.md |
| .devcontainer/devcontainer.json | Added nvmVersion: "latest" to node feature configuration to ensure nvm availability in development containers |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| echo ""; \ | ||
| echo "Node.js and npm are required but not installed."; \ | ||
| echo "Please install Node.js (version 24 or later recommended) using one of these methods:"; \ | ||
| echo ""; \ | ||
| echo "Using nvm (recommended):"; \ | ||
| echo " curl -o- https://raw.githubusercontent.com/nvm-sh/nvm/v0.40.0/install.sh | bash"; \ | ||
| echo " source ~/.bashrc # or source ~/.zshrc"; \ | ||
| echo " nvm install 24"; \ | ||
| echo ""; \ | ||
| echo "On Ubuntu/Debian:"; \ | ||
| echo " curl -fsSL https://deb.nodesource.com/setup_24.x | sudo -E bash -"; \ | ||
| echo " sudo apt-get install -y nodejs"; \ | ||
| echo ""; \ | ||
| echo "On macOS with Homebrew:"; \ | ||
| echo " brew install node"; \ | ||
| echo ""; \ | ||
| echo "For other platforms, see: https://nodejs.org/en/download/package-manager"; \ | ||
| echo ""; \ |
There was a problem hiding this comment.
[nitpick] The error message spans many lines (lines 124-142) which makes the Makefile harder to read and maintain. Following the pattern from the golint target (lines 165-174), consider extracting the installation instructions to a smaller, more focused message or using a here-document for better readability.
Example approach similar to golint:
@if command -v node >/dev/null 2>&1 && command -v npm >/dev/null 2>&1; then \
npm install -g prettier; \
else \
echo "Node.js and npm are required but not installed."; \
echo "Install Node.js 24+ using nvm (recommended), package manager, or see:"; \
echo " https://nodejs.org/en/download/package-manager"; \
exit 1; \
fiThis maintains the fail-fast behavior while keeping the Makefile more maintainable.
| echo ""; \ | |
| echo "Node.js and npm are required but not installed."; \ | |
| echo "Please install Node.js (version 24 or later recommended) using one of these methods:"; \ | |
| echo ""; \ | |
| echo "Using nvm (recommended):"; \ | |
| echo " curl -o- https://raw.githubusercontent.com/nvm-sh/nvm/v0.40.0/install.sh | bash"; \ | |
| echo " source ~/.bashrc # or source ~/.zshrc"; \ | |
| echo " nvm install 24"; \ | |
| echo ""; \ | |
| echo "On Ubuntu/Debian:"; \ | |
| echo " curl -fsSL https://deb.nodesource.com/setup_24.x | sudo -E bash -"; \ | |
| echo " sudo apt-get install -y nodejs"; \ | |
| echo ""; \ | |
| echo "On macOS with Homebrew:"; \ | |
| echo " brew install node"; \ | |
| echo ""; \ | |
| echo "For other platforms, see: https://nodejs.org/en/download/package-manager"; \ | |
| echo ""; \ | |
| echo "Node.js and npm are required but not installed."; \ | |
| echo "Please install Node.js (version 24 or later recommended). See:"; \ | |
| echo " https://nodejs.org/en/download/package-manager"; \ |
The
make depstarget silently fails in environments without Node.js, causingprettierto install but be non-functional, leading to cryptic "node: not found" errors duringmake deps-dev.Changes
Makefile: Added Node.js/npm availability check before installing prettier
golinttargetDocumentation: Added Node.js 24+ to prerequisites in CONTRIBUTING.md and DEVGUIDE.md
Devcontainer: Added explicit nvm configuration to
.devcontainer/devcontainer.jsonnvmVersion: "latest"to ensure nvm is available in the development containerError Message
Fixes #4160
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.