child_process: ignore undef/proto values of env

[Gerhut]

At present, undefined values of env option will be transferred as an "undefined" string value and values in the prototype will also be included, which are not usual behaviors.

Since non-string env values & prototype values are undocumented, this change may be treated as a bugfix or a breaking change.

Tested on Mac, Windows not yet.

Fixes: #15087

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

Affected core subsystem(s)

child_process

[benjamingr]

I think this should optimally be:

constenvPairs=Object.keys(env).filter(Boolean).map(key=>`${key}=${env[key]}`)

or something similar (ignore empty keys in addition to undefined ones.

I do think it's underspecified though. So I'll wait for someone who understands it better to weigh in.

[refack]

OMG .filter(Boolean) 💘
I used to do .filter((x) => x), but have alway looked for a built-in.

P.S. @Gerhut in TF&I (node 8.3.0 and newer) for ... of loop are almost 10 time faster than for ... in loops, and it's supports Object.entries()

[benjamingr]

@refack but the call to Object.keys is itself about as slow as doing for... in - in this particular case I don't think performance is important given the time it takes to create a process and all 😅

[refack]

@benjamingr agreed.
I'm just promoting TF&I good behaviour (no more CranksharfScript)

[Gerhut]

Since the code around still in legacy Syntax, should I be allowed to use modern syntax? If so I will modify them in modern syntax.

[benjamingr]

@Gerhut yes, in general new code should be written in new syntax.

[Gerhut]

However, for..in includes keys in prototype, and keys-in-prototype of env test exists at https://github.com/nodejs/node/blob/master/test/parallel/test-child-process-env.js#L31-L33 . Changing to Object.keys/entries will fail them

[BridgeAR]

I think it would be good to remove that test case and only use Object.keys. There is not much reasoning in the original commit about why it was added but I think this is actually a faulty behavior.

[BridgeAR]

Using filter(Boolean) would be a bad decision to use though as it would remove a empty string and any other falsy value as well.

[benjamingr]

Tagging as semver-major preemptively since this is a user facing behavioral change in an API marked 2 - Stable.

I don't feel strongly about it - I just don't want it to be missed.

[benjamingr]

@Gerhut thank you for the contribution. Just to explain the process at this point:

• Collaborators are going to look at this pull request for around 48 hours to discuss the change.
• People will comment on the actual code and might suggest changes.
• People will comment on the merit of the actual changes and evaluate how they impact the ecosystem.

If at any point you would like assistance with the pull request or have any question about the process feel free to ask here. Welcome.

[cjihrig]

Can you add documentation notes and function level changelog entries for this.

[addaleax]

LGTM as semver-major

[BridgeAR]

I personally am not convinced about this. Why should undefined and null be treated otherwise than any other primitive besides strings? They will all be coerced to a string and I can actually imagine that some people might explicitly pass undefined or null as env value. Having a implicit removal is not what I would call intuitive.
I would rather have a error thrown as a user to know what is happening and that it is better to always pass in values as string.

[jasnell]

I'm kind of half-way where @BridgeAR is. I would say null should be ToString'd and undefined should be ignored/omitted.

[targos]

I think it makes sense to ignore undefined too. No opinion on null.

[addaleax]

100 % totally official twitter poll: https://twitter.com/addaleax/status/903796395865452544

[BridgeAR]

Looks like the poll says null should be stringified and undefined should be removed. @Gerhut would you be so kind and update the PR accordingly?

I personally still think we should not remove it but throw an error instead and the poll did not cover that part but it seems like most people agree on removing it, so I will not hold this off.

[refack]

BTW: need to think about how to make this consistent with assignment (maybe in a future PR)

node/doc/api/process.md

Lines 827 to 842 in 98d8db3

	Assigning a property on `process.env` will implicitly convert the value
	to a string.
	Example:
	```js
	process.env.test=null;
	console.log(process.env.test);
	// => 'null'
	process.env.test=undefined;
	console.log(process.env.test);
	// => 'undefined'
	```
	Use `delete` to delete a property from `process.env`.

[Gerhut]

PR Updated.

Moreover, Would "ignore keys in prototype of env option" be another breaking change of bugfix? (See outdated review comment)

I just feel this change should be in another PR to discuss separately. There could be an use case that using process.env as a base environment variables and extend it in env option. It might using prototyping (env = Object.create(process.env); env.FOO='BAR'; spawn(exe, {env};).

[BridgeAR]

In general LGTM but I would like my comment to be addressed.

[BridgeAR]

I am not sure how fast Object.entries is and I somewhat expect it to be slower than e.g. using Object.keys or using for ... in with a Object.hasOwnProperty.call(env, key) check. The latter will definitely be faster in upcoming v8 versions but probably not right now.

[BridgeAR]

@Gerhut I think it is fine to keep it in this PR but it would be just as fine to move it to another one!

If this is kept in the PR the commit message has to be updated though. This has to be done one way or the other anyway though since it currently has information that is not necessary for landing. So I think it is your call. If you want to land another commit - feel free to open a second PR for that.

[bnoordhuis]

I'm -0. process.env and the .env option currently behave the same: values are tostring-ed. This PR introduces a discrepancy for no good reason I can see.

Dimissed my ok as I am also -0 on the change but I do not want to block it either.

[BridgeAR]

This is semver major and needs some LGs @nodejs/tsc

It is still a tiny bit controversial in general though, so please read all the comments as well.

[evanlucas]

I'm on the fence on this one. Deviating from how null and undefined are handled in process.env makes this more confusing to me...

[targos]

I would be in favour of the same change for process.env

[refack]

Could you add 'EMPTY': ''

[refack]

Suggestion keep the old line, and add a new sentence:

process, the default is [`process.env`][].
`undefined` values in `env` will be ignored.

[addaleax]

Can you make sure running make lint passes?

[Gerhut]

Sorry for only ran make test-parallel these times.

[targos]

CI before landing: https://ci.nodejs.org/job/node-test-pull-request/12531/

[targos]

Pushed a fix for Windows.
CI: https://ci.nodejs.org/job/node-test-commit-windows-fanned/14890/

[targos]

Landed in 85739b6.
Thank you @Gerhut for your patience and first contribution to Node!

[MylesBorins]

In future please run CITGM before landing semver major commits

edit: for context this broke CITGM

[targos]

Sorry about that

[apapirovski]

I'll just leave a note here to anyone landing this in the future, it can't land without #18210.