-
Notifications
You must be signed in to change notification settings - Fork 81
Support nvm via .nvmrc file detection #1610
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support nvm via .nvmrc file detection #1610
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| // If .nvmrc exists, run 'nvm use' to load the correct Node.js version. | ||
| // We use a conditional command that only runs 'nvm use' if nvm is available, | ||
| // preventing errors in environments where nvm is not installed. | ||
| const nvmCommand = hasNvmrc ? "type nvm > /dev/null 2>&1 && nvm use; " : ""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the reason we only call nvm use when the file is present? given we can handle the case the commend throws, maybe it'd be better to just call it here instead of testing agains the .nvmrc file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're absolutely right. I've removed the .nvmrc file check since nvm use itself handles the case gracefully when the file is not present. The command now always runs type nvm > /dev/null 2>&1 && nvm use; which is simpler and works correctly in all scenarios.
Changes in commit 1a53e76
filip131311
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot what is the reason we only call nvm use when the file is present? given we can handle the case the commend throws, maybe it'd be better to just call it here instead of testing agains the .nvmrc file
| // If .nvmrc exists, run 'nvm use' to load the correct Node.js version. | ||
| // We use a conditional command that only runs 'nvm use' if nvm is available, | ||
| // preventing errors in environments where nvm is not installed. | ||
| const nvmCommand = hasNvmrc ? "type nvm > /dev/null 2>&1 && nvm use; " : ""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot I think it'd be better to dry-run nvm use once to check if it's set up correctly, and then add it to the execa call below only if successful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good suggestion! I've implemented the dry-run approach. Now nvm use is tested first with an execa call, and only included in the actual PATH command if it succeeds. This means projects without .nvmrc won't see error messages, and systems without nvm won't attempt to use it.
Changes in commit c0ef927
Good point! I've simplified the implementation to always call Changes in commit 1a53e76 |
| "-l", | ||
| "-c", | ||
| `cd "${appRoot}" && echo "${RNIDE_PATH_DELIMITER}$PATH${RNIDE_PATH_DELIMITER}"`, | ||
| `cd "${appRoot}" && ${nvmCommand}echo "${RNIDE_PATH_DELIMITER}$PATH${RNIDE_PATH_DELIMITER}"`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot since the nvmCommand ends with ;, won't that change the behavior to not fail if cd appRoot didn't succeed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're absolutely right! The semicolon breaks the error chain. With ;, the echo command would run even if cd fails. I've changed it to use && instead, so the entire command chain properly fails if cd doesn't succeed.
Changes in commit 07a1cf8
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot can you change this so that
- you generate a list of commands to run in the chain
- you always have
cdandechocommands on the list - you only include
nvm useifnvmis available - you generate the final command chain by calling
.join(" && ")on that list?
I think it would be more readable that way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Refactored to use a commands array that always includes cd and echo, conditionally adds nvm use if available, and generates the final command with .join(" && ").
Changes in commit e52ae3c
|
@copilot can you rebase this branch onto main? |
I'm unable to rebase this branch as I don't have the ability to perform git rebase operations or force push. You'll need to rebase this branch manually, or I can help with other changes if needed. |
jwajgelt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I don't have an up to date assesment of this PR but it seems to be blocked by my old review so i dismiss it
Found some issues, will fix them manually
Co-authored-by: kmagiera <[email protected]>
Co-authored-by: filip131311 <[email protected]>
Co-authored-by: jwajgelt <[email protected]>
Co-authored-by: jwajgelt <[email protected]>
Co-authored-by: jwajgelt <[email protected]>
853377b to
3d3cfb8
Compare
jwajgelt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to work fine now.
Adds support for nvm via
.nvmrcfile detection. Some projects (i.e. bsky) use.nvmrcfile to specify the node version. Currently, the only way to make Radon honor that file would be to go to the project directory, runnvm useand then launch code from the terminal. This isn't ideal especially for people who work on a couple of different projects with different node version requirements.This change runs
nvm usesimilarly to how we initialize PATH variables to support tools like mise.Fixes #1609
Changes Made
Modified
packages/vscode-extension/src/utilities/subprocess.ts:commandsarray with.join(" && ")for better readability:cdto app root is always includednvm useis conditionally added only if the dry-run succeedsechoto capture PATH is always includedImplementation Details
The solution uses a dry-run approach to optimize performance and avoid unnecessary error messages:
nvm usein the application root directorynvm usein the actual PATH command if the dry-run succeeds.nvmrcfile exists, the dry-run fails and nvm is skipped entirely.join(" && ")for clear, maintainable codeThis means:
.nvmrc→ nvm use runs and loads the correct version ✅.nvmrc→ nvm is skipped (no error messages in logs) ✅How Has This Been Tested:
.nvmrcpresent → nvm use is included.nvmrc→ nvm use is skippedcdfails, the entire command chain stops (correct behavior)cdsucceeds,nvm useruns (if available), thenechoruns (correct behavior)&&chainingHow Has This Change Been Documented:
N/A - This is an internal enhancement that automatically detects and uses
.nvmrcfiles when present, requiring no user documentation.Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.