src: merge env-file and env vars of NODE_OPTIONS

[anonrig]

Follow-up for #49148. Prior to this change, we were preferring the environment variables over .env file. Right now, we merge them.

Ref: #49148

cc @GeoffreyBooth @ljharb @tniessen @gireeshpunathil

[nodejs-github-bot]

Review requested:

• @nodejs/startup

[tniessen]

I am a little confused. Wasn't the opinion in #49148 that merging would be confusing and inconsistent with how the implementation treats other environment variables? Or does merging refer to something else here?

[anonrig]

I am a little confused. Wasn't the opinion in #49148 that merging would be confusing and inconsistent with how the implementation treats other environment variables? Or does merging refer to something else here?

Referring to @targos's comment on the original PR (https://github.com/nodejs/node/pull/48890/files#r1294239790), it sort of makes sense to me to have an edge case for NODE_OPTIONS. The only case I can think of this makes sense is, people who have NODE_OPTIONS in their shell, will always override the NODE_OPTIONS settings in .env files, and might not even know why their code doesn't work.

[ljharb]

I think the primary reason this can't work out well is that if one of the sources has --foo, there's not always going to be a --no-foo i can put in the other source to override it. In other words, without the overriding behavior, i wouldn't be able to prevent the other source's NODE_OPTIONS from remaining.

[GeoffreyBooth]

I think it’s too confusing to special-case NODE_OPTIONS for merging when all other variable names are overwritten. It’s even weirder because there are Node environment variables that we can’t merge, like FORCE_COLOR=[1, 2, 3] or NODE_EXTRA_CA_CERTS=file. So the special case wouldn’t be “Node-supported environment variables are merged” but specifically only NODE_OPTIONS is.

I think the “proper” way to support merging is to support some kind of substitution syntax in the file, like

NODE_OPTIONS=${NODE_OPTIONS} --no-warnings

Then the user can explicitly control the merging, for any variable, and define how it’s done (is the file’s value prepended or appended, etc.).

[anonrig]

I'm convinced by your arguments. I've added blocked label to not merge this. I'll keep the pull request open for a couple of days, in case someone with a good argument comes in. If not, I'll close it soon. Thanks everybody for their feedback and review.