FR: child_process.exec(cmd, { additionalEnv })

[refack]

• Version: *
• Platform: *
• Subsystem: child_process

child_process.exec(cmd, { env: Object.assign({}, process.env, {NEW_VAR:1}) }) is a very common pattern. IMHO adding an { additionalEnv } option that implements this pattern, will make the API more complete, and less error prone.

[vsemozhetbyt]

FWIW, in Node.js 8.3.0 we can do:

child_process.exec(cmd, { env: { ...process.env, NEW_VAR: 1 } });

[cjihrig]

I've thought about this in the past. It seems like an awkward API to me though. I like @vsemozhetbyt's solution moving forward, and Object.assign() (or similar) for older versions.

[refack]

Ack that object spreads make this more succinct.
IMHO there is still the issue that the current API is too close the the POSIX/WIN32 system call, and is not intuitive at the application level.
I think that the case of adding vars is much more common than the case of replacing the entire env...
Maybe a cleaner option would be to add a { envAdditive: true } flag (with a default of false) that will treat env as additions instead of replacement.

[bnoordhuis]

I think that the case of adding vars is much more common than the case of replacing the entire env...

C programmers will disagree with you.

This feature request seems like a solution in search of a problem and as to the syntax, I agree with Colin's choice of words: awkward.

[refack]

I think that the case of adding vars is much more common than the case of replacing the entire env...

C programmers will disagree with you.

This feature request seems like a solution in search of a problem and as to the syntax, I agree with Colin's choice of words: awkward.

That's sort of my point that (C programmes) != (node programmers).
The struggle is real #14822 + #13390, and that's node core code written (or at least reviewed) by node core coders.

I don't have an elegant solution for syntax. Since the API was designed to mimic the system calls, everything would look awkward 🤷‍♂️ The most elegant solution would have been to change the semantics of { env } but that's out of the question.
But I am trying to think what a programmer who is node-as-first-language would think...

[refack]

🤔 trying to be creative:

const aes = child_process.addativeEnvSymbol;
const env = { [aes]: true, NEW_VAR:1 }
child_process.exec(cmd, { env })

[joaolucasl]

The spread operator is a pretty good solution for this, imho, but I like the idea of an option to ease the work of merging too.

I thought of something like:

const env = { /* ... */ };
child_process.exec(cmd, { env, mergeEnvs: true });

mergeEnvs <boolean> If true, merges the env property with process.env. (Default: false).

The naming might be tricky to get right, since to be descriptive enough it would have to be longer lol

Edit: (Happy to PR if this goes forward 😄)

[evanlucas]

I feel like this is something that could easily be done in an npm package

[refack]

After reading some POSIX design stories, I tend to agree that this should stay as is for child_process.spawn but now I'm worried about the mismatch with cluster.fork

[gibfahn]

I think a common function that adds something to the existing process.env like:

child_process.exec(cmd, { env: common.envPlus({NEW_VAR:1}) })

would make sense, and would hopefully solve the problem we have in tests, which is that adding something to the existing env isn't exactly intuitive.

Whether this is something that the larger community would need is a different question though, and can probably be dealt with separately.

[cjihrig]

Would common.envPlus() just be this?

function envPlus(env) {
  return Object.assign({}, process.env, env);
}

If so, we could probably live without it.

[refack]

For test/common I was thinking of common.exec as there might be a wider pattern in common use, but it requires an audit.

[sindresorhus]

I feel like this is something that could easily be done in an npm package

Check out execa which already does this by default. (extendEnv option to turn it off)

[gibfahn]

Raised a PR for discussion purposed: #14845

Not 100% sold on having the function, but it's easier to discuss over code.

[refack]

Closed for no consensus