fix: invalid Node.js version in use-node-version should not break (#9285)

close #9276

.changeset/shaky-stars-hear.md

@@ -0,0 +1,6 @@
+---
+"pnpm": patch
+"@pnpm/plugin-commands-env": patch
+---
+
+Invalid Node.js version in `use-node-version` should not cause pnpm itself to break [#9276](https://github.com/pnpm/pnpm/issues/9276).

env/plugin-commands-env/src/node.ts

@@ -4,14 +4,14 @@ import util from 'util'
 import { type Config } from '@pnpm/config'
 import { getSystemNodeVersion } from '@pnpm/env.system-node-version'
 import { createFetchFromRegistry, type FetchFromRegistry } from '@pnpm/fetch'
-import { globalInfo } from '@pnpm/logger'
+import { globalInfo, globalWarn } from '@pnpm/logger'
 import { fetchNode } from '@pnpm/node.fetcher'
 import { getStorePath } from '@pnpm/store-path'
 import { type PrepareExecutionEnvOptions, type PrepareExecutionEnvResult } from '@pnpm/types'
 import loadJsonFile from 'load-json-file'
 import writeJsonFile from 'write-json-file'
 import { getNodeMirror } from './getNodeMirror'
-import { parseNodeSpecifier } from './parseNodeSpecifier'
+import { isValidVersion, parseNodeSpecifier } from './parseNodeSpecifier'
 
 export type NvmNodeCommandOptions = Pick<Config,
 | 'bin'
@@ -62,7 +62,16 @@ export async function prepareExecutionEnv (config: NvmNodeCommandOptions, { extr
 export async function getNodeBinDir (opts: NvmNodeCommandOptions): Promise<string> {
   const fetch = createFetchFromRegistry(opts)
   const nodesDir = getNodeVersionsBaseDir(opts.pnpmHomeDir)
-  let wantedNodeVersion = opts.useNodeVersion ?? (await readNodeVersionsManifest(nodesDir))?.default
+  const manifestNodeVersion = (await readNodeVersionsManifest(nodesDir))?.default
+  let wantedNodeVersion = opts.useNodeVersion ?? manifestNodeVersion
+  if (opts.useNodeVersion != null) {
+    // If the user has specified an invalid version via use-node-version, we should not throw an error. Or else, it will break all the commands.
+    // Instead, we should fallback to the manifest node version
+    if (!isValidVersion(opts.useNodeVersion)) {
+      globalWarn(`"${opts.useNodeVersion}" is not a valid Node.js version.`)
+      wantedNodeVersion = manifestNodeVersion
+    }
+  }
   if (wantedNodeVersion == null) {
     const response = await fetch('https://registry.npmjs.org/node')
     wantedNodeVersion = (await response.json() as any)['dist-tags'].lts // eslint-disable-line

env/plugin-commands-env/src/parseNodeSpecifier.ts

@@ -5,9 +5,25 @@ export interface NodeSpecifier {
   useNodeVersion: string
 }
 
-const isStableVersion = (version: string) => /^\d+\.\d+\.\d+$/.test(version)
+const isStableVersion = (version: string): boolean => /^\d+\.\d+\.\d+$/.test(version)
+const matchPrereleaseVersion = (version: string): RegExpMatchArray | null => version.match(/^\d+\.\d+\.\d+-(nightly|rc|test|v8-canary)(\..+)$/)
+
 const STABLE_RELEASE_ERROR_HINT = 'The correct syntax for stable release is strictly X.Y.Z or release/X.Y.Z'
 
+export function isValidVersion (specifier: string): boolean {
+  if (specifier.includes('/')) {
+    const [releaseChannel, useNodeVersion] = specifier.split('/')
+
+    if (releaseChannel === 'release') {
+      return isStableVersion(useNodeVersion)
+    }
+
+    return useNodeVersion.includes(releaseChannel)
+  }
+
+  return isStableVersion(specifier) || matchPrereleaseVersion(specifier) != null
+}
+
 export function parseNodeSpecifier (specifier: string): NodeSpecifier {
   if (specifier.includes('/')) {
     const [releaseChannel, useNodeVersion] = specifier.split('/')
@@ -25,7 +41,7 @@ export function parseNodeSpecifier (specifier: string): NodeSpecifier {
     return { releaseChannel, useNodeVersion }
   }
 
-  const prereleaseMatch = specifier.match(/^\d+\.\d+\.\d+-(nightly|rc|test|v8-canary)(\..+)$/)
+  const prereleaseMatch = matchPrereleaseVersion(specifier)
   if (prereleaseMatch != null) {
     return { releaseChannel: prereleaseMatch[1], useNodeVersion: specifier }
   }

env/plugin-commands-env/test/node.test.ts

@@ -1,8 +1,10 @@
 import AdmZip from 'adm-zip'
 import { Response } from 'node-fetch'
 import path from 'path'
+import fs from 'fs'
 import { Readable } from 'stream'
 import tar from 'tar-stream'
+import { globalWarn } from '@pnpm/logger'
 import {
   getNodeDir,
   getNodeBinDir,
@@ -34,8 +36,17 @@ jest.mock('@pnpm/fetch', () => ({
   createFetchFromRegistry: () => fetchMock,
 }))
 
+jest.mock('@pnpm/logger', () => {
+  const originalModule = jest.requireActual('@pnpm/logger')
+  return {
+    ...originalModule,
+    globalWarn: jest.fn(),
+  }
+})
+
 beforeEach(() => {
   fetchMock.mockClear()
+  ;(globalWarn as jest.Mock).mockClear()
 })
 
 test('check API (placeholder test)', async () => {
@@ -94,6 +105,28 @@ test('get node version base dir', async () => {
   expect(versionDir).toBe(path.resolve(process.cwd(), 'nodejs'))
 })
 
+test('specified an invalid Node.js via use-node-version should not cause pnpm itself to break', async () => {
+  tempDir()
+  const configDir = path.resolve('config')
+
+  const opts: NvmNodeCommandOptions = {
+    bin: process.cwd(),
+    configDir,
+    global: true,
+    pnpmHomeDir: process.cwd(),
+    rawConfig: {},
+    useNodeVersion: '22.14',
+  }
+
+  fs.mkdirSync('nodejs', { recursive: true })
+  fs.writeFileSync('nodejs/versions.json', '{"default":"16.4.0"}', 'utf8')
+
+  expect(await getNodeBinDir(opts)).toBeTruthy()
+
+  const calls = (globalWarn as jest.Mock).mock.calls
+  expect(calls[calls.length - 1][0]).toContain('"22.14" is not a valid Node.js version.')
+})
+
 describe('prepareExecutionEnv', () => {
   test('should not proceed to fetch Node.js if the process is already running in wanted node version', async () => {
     fetchMock.mockImplementationOnce(() => {

env/plugin-commands-env/test/parseNodeSpecifier.ts

@@ -1,4 +1,4 @@
-import { parseNodeSpecifier } from '../lib/parseNodeSpecifier'
+import { isValidVersion, parseNodeSpecifier } from '../lib/parseNodeSpecifier'
 
 test.each([
   ['rc/16.0.0-rc.0', '16.0.0-rc.0', 'rc'],
@@ -43,3 +43,27 @@ test.each([
   await expect(promise).rejects.toThrow(`"${specifier}" is not a valid Node.js version`)
   await expect(promise).rejects.toHaveProperty('hint', 'The correct syntax for stable release is strictly X.Y.Z or release/X.Y.Z')
 })
+
+test.each([
+  ['rc/16.0.0-rc.0', '16.0.0-rc.0', 'rc'],
+  ['16.0.0-rc.0', '16.0.0-rc.0', 'rc'],
+  ['release/16.0.0', '16.0.0', 'release'],
+  ['16.0.0', '16.0.0', 'release'],
+])('valid Node.js specifier', async (specifier) => {
+  expect(isValidVersion(specifier)).toBe(true)
+})
+
+test.each([
+  ['nightly'],
+  ['rc'],
+  ['test'],
+  ['v8-canary'],
+  ['release'],
+  ['stable'],
+  ['latest'],
+  ['release/16.0.0.release.0'],
+  ['16'],
+  ['16.0'],
+])('invalid Node.js specifier', async (specifier) => {
+  expect(isValidVersion(specifier)).toBe(false)
+})