Documentation: Include specific nvm version needed #33
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## trunk #33 +/- ##
=========================================
Coverage 88.43% 88.43%
Complexity 94 94
=========================================
Files 8 8
Lines 519 519
=========================================
Hits 459 459
Misses 60 60
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Hey @rollybueno
Not entirely sure I understand your concern here ( npm install will fail if Node.js is not installed too, regardless of if someone uses NVM), so take the following feedback with a grain of salt, but I have misgivings about the solution in this PR.
Unlike nvm install which is meant to install the needed version once, nvm use is meant to be used every time you spin up a local setup (pull/switch branches etc).
That's why the node version is listed as a prereq (like Composer and Docker), while nvm use is included in the instructions alongside npm install.
If there's a need to clarify how to use NVM to install node versions, I strongly suggest we find a different spot for it and not inside this bash script (similar to how we dont include a cd path/to/abilities-api) in this particular part of the docs..
Smaller feedback on the changes in the diff
|
Sorry I've been away for a while and now just got back. I have encountered an error when I first run the original command since I don't have v20+ in my other laptop. I'm aware we have it under prerequisite, but the idea is to have a single command to install all dependencies in one go.. It's not a real blocker but a very small "improvement" for new contributors. If we think that the info under prerequisite is good enough and let the contributors to use existing command, we can close this. |
gziolo
left a comment
There was a problem hiding this comment.
Let's land this small tweak that might help with contributing 👍🏻
Current issue: The nvm use command will fail if Node.js 20.10.0+ isn't already installed, causing setup errors for new contributors.
Solution: Specify the minimum version requirement and provide a single command that handles both installation and activation.
Issue: #32