Skip to content

Commit

Permalink
fix(core): expand env variables on load and unload (#26459)
Browse files Browse the repository at this point in the history 
This pr is meant to replace #22585 and
#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 #23581

Co-authored-by: Mateo Tibaquira <[email protected]>
(cherry picked from commit 88fd03b)
  • Loading branch information
@xiongemi @FrozenPandaz
xiongemi authored and FrozenPandaz committed Jun 26, 2024
1 parent 3abd352 commit ef3d1e2
Show file tree
Hide file tree
Showing 7 changed files with 182 additions and 104 deletions.
86 changes: 50 additions & 36 deletions e2e/nx/src/extras.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import { parseJson } from '@nx/devkit';
import {
checkFilesExist,
cleanupProject,
getSelectedPackageManager,
isNotWindows,
newProject,
readFile,
Expand Down Expand Up @@ -293,49 +292,64 @@ describe('Extra Nx Misc Tests', () => {
});

describe('Env File', () => {
it('should have the right env', () => {
const appName = uniq('app');
const libName = uniq('lib');

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"
NX_USERNAME=$FIRSTNAME $LASTNAME`
);
updateFile(
`apps/${appName}/src/app/app.tsx`,
`
import NxWelcome from './nx-welcome';
export function App() {
return (
<>
<NxWelcome title={process.env.NX_USERNAME} />
</>
);
}
export default App;
`
LASTNAME="lastname"
NX_USERNAME=$FIRSTNAME $LASTNAME`
);
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();
updateJson(join('libs', libName, 'project.json'), (config) => {
config.targets.echo = {
command: 'echo $NX_USERNAME',
};
return config;
});
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');
});
`
);
const unitTestsOutput = runCLI(`test ${appName}`);
expect(unitTestsOutput).toContain('Successfully ran target test');

it('should work with custom env file', () => {
updateFile(`libs/${libName}/.custom1.env`, `hello="hello1"`);
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');
});
});

Expand Down
4 changes: 2 additions & 2 deletions package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -160,8 +160,8 @@
"cz-git": "^1.4.0",
"czg": "^1.4.0",
"detect-port": "^1.5.1",
"dotenv": "~16.3.1",
"dotenv-expand": "^10.0.0",
"dotenv": "~16.4.5",
"dotenv-expand": "~11.0.6",
"ejs": "^3.1.7",
"enhanced-resolve": "^5.8.3",
"esbuild": "0.19.5",
Expand Down
4 changes: 2 additions & 2 deletions packages/nx/package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,8 @@
"cli-cursor": "3.1.0",
"cli-spinners": "2.6.1",
"cliui": "^8.0.1",
"dotenv": "~16.3.1",
"dotenv-expand": "~10.0.0",
"dotenv": "~16.4.5",
"dotenv-expand": "~11.0.6",
"enquirer": "~2.3.6",
"figures": "3.2.0",
"flat": "^5.0.2",
Expand Down
30 changes: 22 additions & 8 deletions packages/nx/src/executors/run-commands/run-commands.impl.spec.ts
View file
Original file line number Diff line number Diff line change
@@ -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, {
Expand Down Expand Up @@ -337,8 +337,6 @@ describe('Run Commands', () => {
result = res;
});

expect(readFile(f)).toEqual('');

setTimeout(() => {
expect(readFile(f)).toEqual('1');
expect(result).toBeNull();
Expand Down Expand Up @@ -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: [
Expand All @@ -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 result = await runCommands(
const f = fileSync().name;
let result = await runCommands(
{
commands: [
{
Expand All @@ -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(
{
Expand Down
47 changes: 32 additions & 15 deletions packages/nx/src/executors/run-commands/run-commands.impl.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 });
if (result.error) {
throw result.error;
}
loadEnvVarsFile(path, env);
} else {
try {
(await import('dotenv')).config();
loadEnvVarsFile('.env', env);
} catch {}
}
}
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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 (
Expand Down Expand Up @@ -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
Expand Down
Loading

0 comments on commit ef3d1e2

Please sign in to comment.