fix(core): expand env variables on load and unload (#26459)

This pr is meant to replace https://github.com/nrwl/nx/pull/22585 and
https://github.com/nrwl/nx/pull/20524

Env variables using other variables were not unloaded from the
environment and further customizations were impossible in more specific
env files.

<!-- Please make sure you have read the submission guidelines before
posting an PR -->
<!--
https://github.com/nrwl/nx/blob/master/CONTRIBUTING.md#-submitting-a-pr
-->

<!-- Please make sure that your commit message follows our format -->
<!-- Example: `fix(nx): must begin with lowercase` -->

## Current Behavior
<!-- This is the behavior we have today -->

## Expected Behavior
<!-- This is the behavior we should expect with the changes in this PR
-->

## Related Issue(s)
<!-- Please link the issue being fixed so it gets closed when this is
merged. -->

Fixes https://github.com/nrwl/nx/issues/23581

Co-authored-by: Mateo Tibaquira <nestormateo@gmail.com>

e2e/nx/src/extras.test.ts

@@ -2,7 +2,6 @@ import { parseJson } from '@nx/devkit';
 import {
   checkFilesExist,
   cleanupProject,
-  getSelectedPackageManager,
   isNotWindows,
   newProject,
   readFile,
@@ -293,49 +292,64 @@ describe('Extra Nx Misc Tests', () => {
   });
 
   describe('Env File', () => {
-    it('should have the right env', () => {
+    const libName = uniq('lib');
-      const appName = uniq('app');
+
+    beforeAll(() => {
       runCLI(
-        `generate @nx/react:app ${appName} --style=css --bundler=webpack --no-interactive`
+        `generate @nx/js:lib ${libName} --bundler=none --unitTestRunner=none --no-interactive`
       );
+    });
+
+    it('should have the right env', () => {
       updateFile(
         '.env',
         `FIRSTNAME="firstname"
-  LASTNAME="lastname"
+LASTNAME="lastname"
-  NX_USERNAME=$FIRSTNAME $LASTNAME`
+NX_USERNAME=$FIRSTNAME $LASTNAME`
       );
-      updateFile(
+      updateJson(join('libs', libName, 'project.json'), (config) => {
-        `apps/${appName}/src/app/app.tsx`,
+        config.targets.echo = {
-        `
+          command: 'echo $NX_USERNAME',
-      import NxWelcome from './nx-welcome';
+        };
-  
+        return config;
-      export function App() {
-        return (
-          <>
-            <NxWelcome title={process.env.NX_USERNAME} />
-          </>
-        );
-      }
-  
-      export default App;
-    `
-      );
-      updateFile(
-        `apps/${appName}/src/app/app.spec.tsx`,
-        `import { render } from '@testing-library/react';
-  
-    import App from './app';
-    
-    describe('App', () => {
-      it('should have a greeting as the title', () => {
-        const { getByText } = render(<App />);
-        expect(getByText(/Welcome firstname lastname/gi)).toBeTruthy();
       });
+      let result = runCLI(`run ${libName}:echo`);
+      expect(result).toContain('firstname lastname');
+
+      updateFile('.env', (content) => {
+        content = content.replace('firstname', 'firstname2');
+        content = content.replace('lastname', 'lastname2');
+        return content;
+      });
+      result = runCLI(`run ${libName}:echo`);
+      expect(result).toContain('firstname2 lastname2');
     });
-  `
+
-      );
+    it('should work with custom env file', () => {
-      const unitTestsOutput = runCLI(`test ${appName}`);
+      updateFile(`libs/${libName}/.custom1.env`, `hello="hello1"`);
-      expect(unitTestsOutput).toContain('Successfully ran target test');
+      updateFile(`libs/${libName}/.custom2.env`, `hello="hello2"`);
+      updateJson(join('libs', libName, 'project.json'), (config) => {
+        config.targets.hello1 = {
+          command: 'echo $hello',
+          options: {
+            envFile: `libs/${libName}/.custom1.env`,
+          },
+        };
+        config.targets.hello2 = {
+          command: 'echo $hello',
+          options: {
+            envFile: `libs/${libName}/.custom2.env`,
+          },
+        };
+        return config;
+      });
+      let result = runCLI(`run ${libName}:hello1`);
+      expect(result).toContain('hello1');
+      result = runCLI(`run ${libName}:hello2`);
+      expect(result).toContain('hello2');
+      result = runCLI(`run-many --target=hello1,hello2`);
+      expect(result).toContain('hello1');
+      expect(result).toContain('hello2');
     });
   });
 

package.json

@@ -160,8 +160,8 @@
     "cz-git": "^1.4.0",
     "czg": "^1.4.0",
     "detect-port": "^1.5.1",
-    "dotenv": "~16.3.1",
+    "dotenv": "~16.4.5",
-    "dotenv-expand": "^10.0.0",
+    "dotenv-expand": "~11.0.6",
     "ejs": "^3.1.7",
     "enhanced-resolve": "^5.8.3",
     "esbuild": "0.19.5",

packages/nx/package.json

@@ -45,8 +45,8 @@
     "cli-cursor": "3.1.0",
     "cli-spinners": "2.6.1",
     "cliui": "^8.0.1",
-    "dotenv": "~16.3.1",
+    "dotenv": "~16.4.5",
-    "dotenv-expand": "~10.0.0",
+    "dotenv-expand": "~11.0.6",
     "enquirer": "~2.3.6",
     "figures": "3.2.0",
     "flat": "^5.0.2",

packages/nx/src/executors/run-commands/run-commands.impl.spec.ts

@@ -1,4 +1,4 @@
-import { readFileSync, unlinkSync, writeFileSync } from 'fs';
+import { appendFileSync, readFileSync, unlinkSync, writeFileSync } from 'fs';
 import { relative } from 'path';
 import { dirSync, fileSync } from 'tmp';
 import runCommands, {
@@ -337,8 +337,6 @@ describe('Run Commands', () => {
           result = res;
         });
 
-        expect(readFile(f)).toEqual('');
-
         setTimeout(() => {
           expect(readFile(f)).toEqual('1');
           expect(result).toBeNull();
@@ -808,7 +806,7 @@ describe('Run Commands', () => {
     });
 
     it('should load the root .env file by default if there is one', async () => {
-      let f = fileSync().name;
+      const f = fileSync().name;
       const result = await runCommands(
         {
           commands: [
@@ -828,8 +826,8 @@ describe('Run Commands', () => {
     it('should load the specified .env file instead of the root one', async () => {
       const devEnv = fileSync().name;
       writeFileSync(devEnv, 'NX_SITE=https://nx.dev/');
-      let f = fileSync().name;
+      const f = fileSync().name;
-      const result = await runCommands(
+      let result = await runCommands(
         {
           commands: [
             {
@@ -843,11 +841,27 @@ describe('Run Commands', () => {
       );
 
       expect(result).toEqual(expect.objectContaining({ success: true }));
-      expect(readFile(f)).toEqual('https://nx.dev/');
+      expect(readFile(f)).toContain('https://nx.dev/');
+
+      appendFileSync(devEnv, 'NX_TEST=$NX_SITE');
+      await runCommands(
+        {
+          commands: [
+            {
+              command: `echo $NX_TEST >> ${f}`,
+            },
+          ],
+          envFile: devEnv,
+          __unparsed__: [],
+        },
+        context
+      );
+      expect(result).toEqual(expect.objectContaining({ success: true }));
+      expect(readFile(f)).toContain('https://nx.dev/');
     });
 
     it('should error if the specified .env file does not exist', async () => {
-      let f = fileSync().name;
+      const f = fileSync().name;
       try {
         await runCommands(
           {

packages/nx/src/executors/run-commands/run-commands.impl.ts

@@ -10,20 +10,29 @@ import {
   PseudoTtyProcess,
 } from '../../tasks-runner/pseudo-terminal';
 import { signalToCode } from '../../utils/exit-codes';
+import {
+  loadAndExpandDotEnvFile,
+  unloadDotEnvFile,
+} from '../../tasks-runner/task-env';
 
 export const LARGE_BUFFER = 1024 * 1000000;
 let pseudoTerminal: PseudoTerminal | null;
 const childProcesses = new Set<ChildProcess | PseudoTtyProcess>();
 
-async function loadEnvVars(path?: string) {
+function loadEnvVarsFile(path: string, env: Record<string, string> = {}) {
+  unloadDotEnvFile(path, env);
+  const result = loadAndExpandDotEnvFile(path, env);
+  if (result.error) {
+    throw result.error;
+  }
+}
+
+function loadEnvVars(path?: string, env: Record<string, string> = {}) {
   if (path) {
-    const result = (await import('dotenv')).config({ path });
+    loadEnvVarsFile(path, env);
-    if (result.error) {
-      throw result.error;
-    }
   } else {
     try {
-      (await import('dotenv')).config();
+      loadEnvVarsFile('.env', env);
     } catch {}
   }
 }
@@ -109,9 +118,6 @@ export default async function (
   terminalOutput: string;
 }> {
   registerProcessListener();
-  if (process.env.NX_LOAD_DOT_ENV_FILES !== 'false') {
-    await loadEnvVars(options.envFile);
-  }
   const normalized = normalizeOptions(options);
 
   if (normalized.readyWhenStatus.length && !normalized.parallel) {
@@ -159,7 +165,8 @@ async function runInParallel(
       true,
       options.usePty,
       options.streamOutput,
-      options.tty
+      options.tty,
+      options.envFile
     ).then((result: { success: boolean; terminalOutput: string }) => ({
       result,
       command: c.command,
@@ -287,11 +294,12 @@ async function runSerially(
         [],
         options.color,
         calculateCwd(options.cwd, context),
-        options.env ?? {},
+        options.processEnv ?? options.env ?? {},
         false,
         options.usePty,
         options.streamOutput,
-        options.tty
+        options.tty,
+        options.envFile
       );
     terminalOutput += result.terminalOutput;
     if (!result.success) {
@@ -321,9 +329,10 @@ async function createProcess(
   isParallel: boolean,
   usePty: boolean = true,
   streamOutput: boolean = true,
-  tty: boolean
+  tty: boolean,
+  envFile?: string
 ): Promise<{ success: boolean; terminalOutput: string }> {
-  env = processEnv(color, cwd, env);
+  env = processEnv(color, cwd, env, envFile);
   // The rust runCommand is always a tty, so it will not look nice in parallel and if we need prefixes
   // currently does not work properly in windows
   if (
@@ -462,13 +471,21 @@ function calculateCwd(
   return path.join(context.root, cwd);
 }
 
-function processEnv(color: boolean, cwd: string, env: Record<string, string>) {
+function processEnv(
+  color: boolean,
+  cwd: string,
+  env: Record<string, string>,
+  envFile?: string
+) {
   const localEnv = appendLocalEnv({ cwd: cwd ?? process.cwd() });
   const res = {
     ...process.env,
     ...localEnv,
     ...env,
   };
+  if (process.env.NX_LOAD_DOT_ENV_FILES !== 'false') {
+    loadEnvVars(envFile, res);
+  }
   // need to override PATH to make sure we are using the local node_modules
   if (localEnv.PATH) res.PATH = localEnv.PATH; // UNIX-like
   if (localEnv.Path) res.Path = localEnv.Path; // Windows

packages/nx/src/tasks-runner/task-env.ts

@@ -121,12 +121,50 @@ function getNxEnvVariablesForTask(
   };
 }
 
-function loadDotEnvFilesForTask(
+/**
-  task: Task,
+ * This function loads a .env file and expands the variables in it.
-  environmentVariables: NodeJS.ProcessEnv
+ * It is going to override existing environmentVariables.
+ * @param filename
+ * @param environmentVariables
+ */
+export function loadAndExpandDotEnvFile(
+  filename: string,
+  environmentVariables: NodeJS.ProcessEnv,
+  override = false
 ) {
+  const myEnv = loadDotEnvFile({
+    path: filename,
+    processEnv: environmentVariables,
+    override,
+  });
+  return expand({
+    ...myEnv,
+    processEnv: environmentVariables,
+  });
+}
+
+/**
+ * This function unloads a .env file and removes the variables in it from the environmentVariables.
+ * @param filename
+ * @param environmentVariables
+ */
+export function unloadDotEnvFile(
+  filename: string,
+  environmentVariables: NodeJS.ProcessEnv,
+  override = false
+) {
+  const parsedDotEnvFile: NodeJS.ProcessEnv = {};
+  loadAndExpandDotEnvFile(filename, parsedDotEnvFile, override);
+  Object.keys(parsedDotEnvFile).forEach((envVarKey) => {
+    if (environmentVariables[envVarKey] === parsedDotEnvFile[envVarKey]) {
+      delete environmentVariables[envVarKey];
+    }
+  });
+}
+
+function getEnvFilesForTask(task: Task): string[] {
   // Collect dot env files that may pertain to a task
-  const dotEnvFiles = [
+  return [
     // Load DotEnv Files for a configuration in the project root
     ...(task.target.configuration
       ? [
@@ -175,39 +213,22 @@ function loadDotEnvFilesForTask(
     `.env.local`,
     `.env`,
   ];
+}
 
+function loadDotEnvFilesForTask(
+  task: Task,
+  environmentVariables: NodeJS.ProcessEnv
+) {
+  const dotEnvFiles = getEnvFilesForTask(task);
   for (const file of dotEnvFiles) {
-    const myEnv = loadDotEnvFile({
+    loadAndExpandDotEnvFile(file, environmentVariables);
-      path: file,
-      processEnv: environmentVariables,
-      // Do not override existing env variables as we load
-      override: false,
-    });
-    environmentVariables = {
-      ...expand({
-        ...myEnv,
-        ignoreProcessEnv: true, // Do not override existing env variables as we load
-      }).parsed,
-      ...environmentVariables,
-    };
   }
-
   return environmentVariables;
 }
 
 function unloadDotEnvFiles(environmentVariables: NodeJS.ProcessEnv) {
-  const unloadDotEnvFile = (filename: string) => {
-    let parsedDotEnvFile: NodeJS.ProcessEnv = {};
-    loadDotEnvFile({ path: filename, processEnv: parsedDotEnvFile });
-    Object.keys(parsedDotEnvFile).forEach((envVarKey) => {
-      if (environmentVariables[envVarKey] === parsedDotEnvFile[envVarKey]) {
-        delete environmentVariables[envVarKey];
-      }
-    });
-  };
-
   for (const file of ['.env', '.local.env', '.env.local']) {
-    unloadDotEnvFile(file);
+    unloadDotEnvFile(file, environmentVariables);
   }
   return environmentVariables;
 }

pnpm-lock.yaml

@@ -555,11 +555,11 @@ devDependencies:
     specifier: ^1.5.1
     version: 1.5.1
   dotenv:
-    specifier: ~16.3.1
+    specifier: ~16.4.5
-    version: 16.3.1
+    version: 16.4.5
   dotenv-expand:
-    specifier: ^10.0.0
+    specifier: ~11.0.6
-    version: 10.0.0
+    version: 11.0.6
   ejs:
     specifier: ^3.1.7
     version: 3.1.8
@@ -9874,7 +9874,7 @@ packages:
       create-require: 1.1.1
       defu: 6.1.4
       destr: 2.0.2
-      dotenv: 16.3.1
+      dotenv: 16.4.5
       git-url-parse: 13.1.1
       is-docker: 3.0.0
       jiti: 1.21.0
@@ -11981,7 +11981,7 @@ packages:
       chalk: 4.1.2
       chokidar: 3.5.3
       cross-spawn: 7.0.3
-      dotenv: 16.3.1
+      dotenv: 16.4.5
       es-module-lexer: 1.4.1
       esbuild: 0.17.6
       esbuild-plugins-node-modules-polyfill: 1.6.1(esbuild@0.17.6)
@@ -17903,7 +17903,7 @@ packages:
     dependencies:
       chokidar: 3.5.3
       defu: 6.1.4
-      dotenv: 16.3.1
+      dotenv: 16.4.5
       giget: 1.2.1
       jiti: 1.21.0
       mlly: 1.5.0
@@ -20090,8 +20090,20 @@ packages:
     engines: {node: '>=12'}
     dev: true
 
-  /dotenv@16.3.1:
+  /dotenv-expand@11.0.6:
-    resolution: {integrity: sha512-IPzF4w4/Rd94bA9imS68tZBaYyBWSCE47V1RGuMrB94iyTOIEwRmVL2x/4An+6mETpLrKJ5hQkB8W4kFAadeIQ==}
+    resolution: {integrity: sha512-8NHi73otpWsZGBSZwwknTXS5pqMOrk9+Ssrna8xCaxkzEpU9OTf9R5ArQGVw03//Zmk9MOwLPng9WwndvpAJ5g==}
+    engines: {node: '>=12'}
+    dependencies:
+      dotenv: 16.4.5
+    dev: true
+
+  /dotenv@16.3.2:
+    resolution: {integrity: sha512-HTlk5nmhkm8F6JcdXvHIzaorzCoziNQT9mGxLPVXW8wJF1TiGSL60ZGB4gHWabHOaMmWmhvk2/lPHfnBiT78AQ==}
+    engines: {node: '>=12'}
+    dev: true
+
+  /dotenv@16.4.5:
+    resolution: {integrity: sha512-ZmdL2rui+eB2YwhsWzjInR8LldtZHGDoQ1ugH85ppHKwpUHL7j7rN0Ti9NCnGiQbhaZ11FpR+7ao1dNsmduNUg==}
     engines: {node: '>=12'}
     dev: true
 
@@ -25118,7 +25130,7 @@ packages:
     engines: {node: '>=14.0.0'}
     dependencies:
       app-root-dir: 1.0.2
-      dotenv: 16.3.1
+      dotenv: 16.4.5
       dotenv-expand: 10.0.0
     dev: true
 
@@ -27704,7 +27716,7 @@ packages:
       cli-cursor: 3.1.0
       cli-spinners: 2.6.1
       cliui: 8.0.1
-      dotenv: 16.3.1
+      dotenv: 16.3.2
       dotenv-expand: 10.0.0
       enquirer: 2.3.6
       figures: 3.2.0
@@ -27769,7 +27781,7 @@ packages:
       cli-cursor: 3.1.0
       cli-spinners: 2.6.1
       cliui: 8.0.1
-      dotenv: 16.3.1
+      dotenv: 16.3.2
       dotenv-expand: 10.0.0
       enquirer: 2.3.6
       figures: 3.2.0