Repository navigation
FR: child_process.exec(cmd, { additionalEnv }) #14823
Description
Activity
FWIW, in Node.js 8.3.0 we can do:
child_process.exec(cmd, { env: { ...process.env, NEW_VAR: 1 } });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.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 treatenvas additions instead of replacement.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.
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...🤔 trying to be creative:
const aes = child_process.addativeEnvSymbol; const env = { [aes]: true, NEW_VAR:1 } child_process.exec(cmd, { env })
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>Iftrue, merges theenvproperty withprocess.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 😄)
I feel like this is something that could easily be done in an npm package
After reading some POSIX design stories, I tend to agree that this should stay as is for
child_process.spawnbut now I'm worried about the mismatch withcluster.forkI think a
commonfunction 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.
Would
common.envPlus()just be this?function envPlus(env) { return Object.assign({}, process.env, env); }
If so, we could probably live without it.
1 remaining item
I feel like this is something that could easily be done in an npm package
Check out
execawhich already does this by default. (extendEnvoption to turn it off)Closed for no consensus
- added a commit that references this issue
on Sep 3, 2017 - added a commit that references this issue
on Sep 5, 2017 - added a commit that references this issue
on Sep 22, 2017 - added 2 commits that reference this issue
on Sep 22, 2017
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.