fix(ng-dev/release): update PATH after nvm install to affect subsequent commands#3659
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances the invokeNvmInstall method to automatically update the PATH environment variable with the Node.js binary location found via nvm which and adds a verification step against the .nvmrc file. Review feedback recommends using path.delimiter for cross-platform compatibility when manipulating the PATH and suggests handling NVM aliases in .nvmrc that are not valid semver ranges to prevent false validation failures.
| import {existsSync} from 'fs'; | ||
| import {join} from 'path'; | ||
| import {existsSync, readFileSync} from 'fs'; | ||
| import {dirname, join} from 'path'; |
There was a problem hiding this comment.
Import delimiter from the path module to ensure the script remains compatible with different operating systems (e.g., Windows uses ; while Unix uses :).
| import {dirname, join} from 'path'; | |
| import {delimiter, dirname, join} from 'path'; |
References
- ng-dev tooling is expected to be compatible with Windows.
| const pathParts = currentPath.split(':'); | ||
|
|
||
| // Only update if the requested node directory is not already the first entry in PATH. | ||
| if (pathParts[0] !== nodeDir) { | ||
| const filteredParts = pathParts.filter((p) => p !== nodeDir); | ||
| process.env['PATH'] = [nodeDir, ...filteredParts].join(':'); |
There was a problem hiding this comment.
Use delimiter instead of a hardcoded : to split and join the PATH environment variable. This ensures compatibility with Windows, as specified in the general rules for this repository.
| const pathParts = currentPath.split(':'); | |
| // Only update if the requested node directory is not already the first entry in PATH. | |
| if (pathParts[0] !== nodeDir) { | |
| const filteredParts = pathParts.filter((p) => p !== nodeDir); | |
| process.env['PATH'] = [nodeDir, ...filteredParts].join(':'); | |
| const pathParts = currentPath.split(delimiter); | |
| // Only update if the requested node directory is not already the first entry in PATH. | |
| if (pathParts[0] !== nodeDir) { | |
| const filteredParts = pathParts.filter((p) => p !== nodeDir); | |
| process.env['PATH'] = [nodeDir, ...filteredParts].join(delimiter); |
References
- ng-dev tooling is expected to be compatible with Windows.
| }); | ||
| const detectedNodeVersion = nodeVersionOutput.trim(); | ||
|
|
||
| if (!semver.satisfies(detectedNodeVersion, nodeVersionFromNvmrc)) { |
There was a problem hiding this comment.
The .nvmrc file can contain NVM aliases (e.g., lts/iron or node) which are not valid semver ranges. Using semver.satisfies directly on such values will return false, causing an unnecessary error. Consider checking if the value is a valid semver range before validating.
| if (!semver.satisfies(detectedNodeVersion, nodeVersionFromNvmrc)) { | |
| const isSemverRange = semver.validRange(nodeVersionFromNvmrc) !== null; | |
| if (isSemverRange && !semver.satisfies(detectedNodeVersion, nodeVersionFromNvmrc)) { |
|
This PR was merged into the repository. The changes were merged into the following branches:
|
2 participants
Updates the PATH environment variable in the parent process after running nvm install, so that subsequent commands spawned by the script use the correct Node.js version.