NODE_OPTIONS updates (#65006)

### What? Previously, parsing and managing the `NODE_OPTIONS` was performed using a series of regular expressions. These were prone to bugs, and have already caused a few issues. This moves us over to the standard `parseArgs` ([docs](https://nodejs.org/docs/latest/api/util.html#utilparseargsconfig)): ```js import { parseArgs } from "node:utils" ``` ### Why? This simplifies the argument parser dramatically, removing the need for any special patterns or accommodations. No need to maintain all these patterns when there's a lightweight built-in parser already available. Fixes #53127 Fixes #53757 Fixes #47083 Fixes #50489 Closes #60919 Closes #59410 Closes NEXT-3219
vercel · Apr 25, 2024 · ab86fcf · ab86fcf
1 parent a6dfe82
commit ab86fcf
Show file tree

Hide file tree

Showing 9 changed files with 282 additions and 77 deletions.
diff --git a/packages/next/src/cli/next-dev.ts b/packages/next/src/cli/next-dev.ts
@@ -4,11 +4,13 @@ import '../server/lib/cpu-profile'
 import type { StartServerOptions } from '../server/lib/start-server'
 import {
   RESTART_EXIT_CODE,
-  checkNodeDebugType,
-  getDebugPort,
+  getNodeDebugType,
+  getParsedDebugAddress,
   getMaxOldSpaceSize,
-  getNodeOptionsWithoutInspect,
+  getParsedNodeOptionsWithoutInspect,
   printAndExit,
+  formatNodeOptions,
+  formatDebugAddress,
 } from '../server/lib/utils'
 import * as Log from '../build/output/log'
 import { getProjectDir } from '../lib/get-project-dir'
@@ -225,23 +227,25 @@ const nextDev = async (
       let resolved = false
       const defaultEnv = (initialEnv || process.env) as typeof process.env
 
-      let NODE_OPTIONS = getNodeOptionsWithoutInspect()
-      let nodeDebugType = checkNodeDebugType()
-
-      const maxOldSpaceSize = getMaxOldSpaceSize()
+      const nodeOptions = getParsedNodeOptionsWithoutInspect()
+      const nodeDebugType = getNodeDebugType()
 
+      let maxOldSpaceSize: string | number | undefined = getMaxOldSpaceSize()
       if (!maxOldSpaceSize && !process.env.NEXT_DISABLE_MEM_OVERRIDE) {
         const totalMem = os.totalmem()
         const totalMemInMB = Math.floor(totalMem / 1024 / 1024)
-        NODE_OPTIONS = `${NODE_OPTIONS} --max-old-space-size=${Math.floor(
-          totalMemInMB * 0.5
-        )}`
+        maxOldSpaceSize = Math.floor(totalMemInMB * 0.5).toString()
+
+        nodeOptions['max-old-space-size'] = maxOldSpaceSize
+
+        // Ensure the max_old_space_size is not also set.
+        delete nodeOptions['max_old_space_size']
       }
 
       if (nodeDebugType) {
-        NODE_OPTIONS = `${NODE_OPTIONS} --${nodeDebugType}=${
-          getDebugPort() + 1
-        }`
+        const address = getParsedDebugAddress()
+        address.port = address.port + 1
+        nodeOptions[nodeDebugType] = formatDebugAddress(address)
       }
 
       child = fork(startServerPath, {
@@ -253,7 +257,7 @@ const nextDev = async (
           NODE_EXTRA_CA_CERTS: startServerOptions.selfSignedCertificate
             ? startServerOptions.selfSignedCertificate.rootCA
             : defaultEnv.NODE_EXTRA_CA_CERTS,
-          NODE_OPTIONS,
+          NODE_OPTIONS: formatNodeOptions(nodeOptions),
         },
       })
 

diff --git a/packages/next/src/lib/helpers/get-registry.ts b/packages/next/src/lib/helpers/get-registry.ts
@@ -1,6 +1,6 @@
 import { execSync } from 'child_process'
 import { getPkgManager } from './get-pkg-manager'
-import { getNodeOptionsWithoutInspect } from '../../server/lib/utils'
+import { getFormattedNodeOptionsWithoutInspect } from '../../server/lib/utils'
 
 /**
  * Returns the package registry using the user's package manager.
@@ -14,7 +14,7 @@ export function getRegistry(baseDir: string = process.cwd()) {
     const output = execSync(`${pkgManager} config get registry`, {
       env: {
         ...process.env,
-        NODE_OPTIONS: getNodeOptionsWithoutInspect(),
+        NODE_OPTIONS: getFormattedNodeOptionsWithoutInspect(),
       },
     })
       .toString()

diff --git a/packages/next/src/lib/worker.ts b/packages/next/src/lib/worker.ts
@@ -1,6 +1,9 @@
 import type { ChildProcess } from 'child_process'
 import { Worker as JestWorker } from 'next/dist/compiled/jest-worker'
-import { getNodeOptionsWithoutInspect } from '../server/lib/utils'
+import {
+  getParsedNodeOptionsWithoutInspect,
+  formatNodeOptions,
+} from '../server/lib/utils'
 type FarmOptions = ConstructorParameters<typeof JestWorker>[1]
 
 const RESTARTED = Symbol('restarted')
@@ -35,18 +38,20 @@ export class Worker {
     this._worker = undefined
 
     const createWorker = () => {
+      // Get the node options without inspect and also remove the
+      // --max-old-space-size flag as it can cause memory issues.
+      const nodeOptions = getParsedNodeOptionsWithoutInspect()
+      delete nodeOptions['max-old-space-size']
+      delete nodeOptions['max_old_space_size']
+
       this._worker = new JestWorker(workerPath, {
         ...farmOptions,
         forkOptions: {
           ...farmOptions.forkOptions,
           env: {
             ...((farmOptions.forkOptions?.env || {}) as any),
             ...process.env,
-            // we don't pass down NODE_OPTIONS as it can
-            // extra memory usage
-            NODE_OPTIONS: getNodeOptionsWithoutInspect()
-              .replace(/--max-old-space-size=[\d]{1,}/, '')
-              .trim(),
+            NODE_OPTIONS: formatNodeOptions(nodeOptions),
           } as any,
         },
       }) as JestWorker

diff --git a/packages/next/src/server/dev/next-dev-server.ts b/packages/next/src/server/dev/next-dev-server.ts
@@ -42,7 +42,7 @@ import { removePathPrefix } from '../../shared/lib/router/utils/remove-path-pref
 import { Telemetry } from '../../telemetry/storage'
 import { type Span, setGlobal, trace } from '../../trace'
 import { findPageFile } from '../lib/find-page-file'
-import { getNodeOptionsWithoutInspect } from '../lib/utils'
+import { getFormattedNodeOptionsWithoutInspect } from '../lib/utils'
 import { withCoalescedInvoke } from '../../lib/coalesced-function'
 import { loadDefaultErrorComponents } from '../load-default-error-components'
 import { DecodeError, MiddlewareNotFoundError } from '../../shared/lib/utils'
@@ -133,7 +133,7 @@ export default class DevServer extends Server {
           // would be started if user launch Next.js in debugging mode. The number of debuggers is linked to
           // the number of workers Next.js tries to launch. The only worker users are interested in debugging
           // is the main Next.js one
-          NODE_OPTIONS: getNodeOptionsWithoutInspect(),
+          NODE_OPTIONS: getFormattedNodeOptionsWithoutInspect(),
         },
       },
     }) as Worker & {

diff --git a/packages/next/src/server/lib/start-server.ts b/packages/next/src/server/lib/start-server.ts
@@ -17,7 +17,11 @@ import os from 'os'
 import Watchpack from 'next/dist/compiled/watchpack'
 import * as Log from '../../build/output/log'
 import setupDebug from 'next/dist/compiled/debug'
-import { RESTART_EXIT_CODE, checkNodeDebugType, getDebugPort } from './utils'
+import {
+  RESTART_EXIT_CODE,
+  getFormattedDebugAddress,
+  getNodeDebugType,
+} from './utils'
 import { formatHostname } from './format-hostname'
 import { initialize } from './router-server'
 import { CONFIG_FILES } from '../../shared/lib/constants'
@@ -211,10 +215,10 @@ export async function startServer(
     }
   })
 
-  const nodeDebugType = checkNodeDebugType()
-
   await new Promise<void>((resolve) => {
     server.on('listening', async () => {
+      const nodeDebugType = getNodeDebugType()
+
       const addr = server.address()
       const actualHostname = formatHostname(
         typeof addr === 'object'
@@ -236,9 +240,9 @@ export async function startServer(
       }://${formattedHostname}:${port}`
 
       if (nodeDebugType) {
-        const debugPort = getDebugPort()
+        const formattedDebugAddress = getFormattedDebugAddress()
         Log.info(
-          `the --${nodeDebugType} option was detected, the Next.js router server should be inspected at port ${debugPort}.`
+          `the --${nodeDebugType} option was detected, the Next.js router server should be inspected at ${formattedDebugAddress}.`
         )
       }
 

diff --git a/.../get-node-options-without-inspect.test.ts → packages/next/src/server/lib/utils.test.ts b/.../get-node-options-without-inspect.test.ts → packages/next/src/server/lib/utils.test.ts
@@ -1,52 +1,68 @@
-/* eslint-env jest */
-import { getNodeOptionsWithoutInspect } from 'next/dist/server/lib/utils'
+import {
+  getFormattedNodeOptionsWithoutInspect,
+  getParsedDebugAddress,
+} from './utils'
 
 const originalNodeOptions = process.env.NODE_OPTIONS
 
 afterAll(() => {
   process.env.NODE_OPTIONS = originalNodeOptions
 })
 
-describe('getNodeOptionsWithoutInspect', () => {
+describe('getParsedDebugAddress', () => {
+  it('supports the flag with an equal sign', () => {
+    process.env.NODE_OPTIONS = '--inspect=1234'
+    const result = getParsedDebugAddress()
+    expect(result).toEqual({ host: undefined, port: 1234 })
+  })
+
+  it('supports the flag without an equal sign', () => {
+    process.env.NODE_OPTIONS = '--inspect 1234'
+    const result = getParsedDebugAddress()
+    expect(result).toEqual({ host: undefined, port: 1234 })
+  })
+})
+
+describe('getFormattedNodeOptionsWithoutInspect', () => {
   it('removes --inspect option', () => {
     process.env.NODE_OPTIONS = '--other --inspect --additional'
-    const result = getNodeOptionsWithoutInspect()
+    const result = getFormattedNodeOptionsWithoutInspect()
 
     expect(result).toBe('--other --additional')
   })
 
   it('removes --inspect option at end of line', () => {
     process.env.NODE_OPTIONS = '--other --inspect'
-    const result = getNodeOptionsWithoutInspect()
+    const result = getFormattedNodeOptionsWithoutInspect()
 
-    expect(result).toBe('--other ')
+    expect(result).toBe('--other')
   })
 
   it('removes --inspect option with parameters', () => {
     process.env.NODE_OPTIONS = '--other --inspect=0.0.0.0:1234 --additional'
-    const result = getNodeOptionsWithoutInspect()
+    const result = getFormattedNodeOptionsWithoutInspect()
 
     expect(result).toBe('--other --additional')
   })
 
   it('removes --inspect-brk option', () => {
     process.env.NODE_OPTIONS = '--other --inspect-brk --additional'
-    const result = getNodeOptionsWithoutInspect()
+    const result = getFormattedNodeOptionsWithoutInspect()
 
     expect(result).toBe('--other --additional')
   })
 
   it('removes --inspect-brk option with parameters', () => {
     process.env.NODE_OPTIONS = '--other --inspect-brk=0.0.0.0:1234 --additional'
-    const result = getNodeOptionsWithoutInspect()
+    const result = getFormattedNodeOptionsWithoutInspect()
 
     expect(result).toBe('--other --additional')
   })
 
   it('ignores unrelated options starting with --inspect-', () => {
     process.env.NODE_OPTIONS =
       '--other --inspect-port=0.0.0.0:1234 --additional'
-    const result = getNodeOptionsWithoutInspect()
+    const result = getFormattedNodeOptionsWithoutInspect()
 
     expect(result).toBe('--other --inspect-port=0.0.0.0:1234 --additional')
   })