Skip to content

fix(ng-dev/release): update PATH after nvm install to affect subsequent commands #3659

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.

Sign up for GitHub

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

Merged
josephperrott merged 1 commit into from
May 12, 2026
+44 −10
Merged

fix(ng-dev/release): update PATH after nvm install to affect subsequent commands #3659

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 44 additions & 10 deletions ng-dev/release/publish/external-commands.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@
* found in the LICENSE file at https://angular.io/license
*/

import {existsSync} from 'fs';
import {join} from 'path';
import {existsSync, readFileSync} from 'fs';
import {dirname, join} from 'path';
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Import delimiter from the path module to ensure the script remains compatible with different operating systems (e.g., Windows uses ; while Unix uses :).

Suggested change
import {dirname, join} from 'path';
import {delimiter, dirname, join} from 'path';
References
  1. ng-dev tooling is expected to be compatible with Windows.

import semver from 'semver';
import {ChildProcess, SpawnResult, SpawnOptions} from '../../utils/child-process.js';
import {Spinner} from '../../utils/spinner.js';
Expand Down Expand Up @@ -193,6 +193,10 @@ export abstract class ExternalCommands {
/**
* Invokes the `nvm install` command in order to install the correct Node.js version
* as specified in a `.nvmrc` file, if present.
*
* We then run `nvm install` to install the correct version of node. We then run
* `nvm which` to get the path to the node binary, and we update the PATH
* environment variable to include this new node binary location.
*/
static async invokeNvmInstall(projectDir: string): Promise<void>;
static async invokeNvmInstall(projectDir: string, quiet: boolean): Promise<void>;
Expand All @@ -202,20 +206,50 @@ export abstract class ExternalCommands {
}

try {
const nodeVersionFromNvmrc = readFileSync(join(projectDir, '.nvmrc'), 'utf8').trim();

// We must source nvm.sh so the shell recognizes the 'nvm' command since nvm is not a binary
// but a shell function. The dot (.) built-in and && operator require shell: true here.
await ChildProcess.spawn('. ~/.nvm/nvm.sh && nvm install', [], {
// We redirect stdout of nvm install to stderr so that stdout only contains the result of nvm which.
const {stdout} = await ChildProcess.spawn(
'. ~/.nvm/nvm.sh && nvm install >&2 && nvm which',
[],
{
cwd: projectDir,
mode: 'on-error',
shell: true,
},
);

const nodeBinPath = stdout.trim();
if (nodeBinPath) {
const nodeDir = dirname(nodeBinPath);
const currentPath = process.env['PATH'] || '';
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(':');
Comment on lines +228 to +233
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
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
  1. ng-dev tooling is expected to be compatible with Windows.

Log.debug(`Updated process.env.PATH to include ${nodeDir}`);
}
}

const {stdout: nodeVersionOutput} = await ChildProcess.spawn('node', ['--version'], {
mode: 'silent',
cwd: projectDir,
mode: 'on-error',
shell: true,
});
const detectedNodeVersion = nodeVersionOutput.trim();

if (!semver.satisfies(detectedNodeVersion, nodeVersionFromNvmrc)) {
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
if (!semver.satisfies(detectedNodeVersion, nodeVersionFromNvmrc)) {
const isSemverRange = semver.validRange(nodeVersionFromNvmrc) !== null;
if (isSemverRange && !semver.satisfies(detectedNodeVersion, nodeVersionFromNvmrc)) {

Log.error(` ✘ Node.js version mismatch after update.\n`);
Log.error(` Expected version: ${nodeVersionFromNvmrc}`);
Log.error(` Actual version: ${detectedNodeVersion}`);
throw new FatalReleaseActionError();
}

if (!quiet) {
const {stdout: nodeVersion} = await ChildProcess.spawn('node', ['--version'], {
mode: 'silent',
cwd: projectDir,
});
Log.info(green(` ✓ Set node version to ${nodeVersion}.`));
Log.info(green(` ✓ Set node version to ${detectedNodeVersion}.`));
}
} catch (e) {
Log.error(e);
Expand Down
Loading