child_process: do not ignore proto values of env

[apapirovski]

This reverts part of the behaviour introduced in a recent PR, and updates the test. Without this change, CitGM and other packages are broken. I would like to have this fast-tracked (like, land within an hour or less) because as things are, CitGM is completely broken.

Also, I would like to propose, that going forward all changes that are labeled as semver-minor/major/patch MUST have a CitGM run to land. If any issues are found, collaborators & TSC should decide whether those are blocking or not. In either case, an effort should be made to fix the impacted modules before the PR lands — or at the very least in cases where those modules are integral to the Node.js core project.

Refs: 85739b6
Fixes: nodejs/citgm#536

Checklist

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

Affected core subsystem(s)

child_process, test

[evanlucas]

Should we not revert this entirely?

[apapirovski]

Should we not revert this entirely?

We don't need to. Just the prototype portion is excluded. To be honest, IMO that portion of the change kind of snuck through because the documented part is only re: undefined values.

[apapirovski]

CI: https://ci.nodejs.org/job/node-test-pull-request/12586/
CitGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1206/

(Does CitGM use the newly built node version to run itself? If not, that might not work so well...)

[joyeecheung]

@apapirovski I think it does because the previous citgm errors seemed to be caused by citgm itself, not the modules that it was testing.

[richardlau]

Note that some of the modules tested by CitGM will fail on master: nodejs/citgm#517

[addaleax]

To be clear, my approval is not just +1 to landing this but also +1 to considering this the right behaviour.

[ljharb]

Why is this conceptually the right behavior? By far, the language conventions when reflecting on objects are for only including own enumerable properties - eg Object.keys/values/entries/assign and object spread.

[mcollina]

LGTM with a nit.

[mcollina]

I would add a comment that flag that states that copying the prototype values is intentional.

[apapirovski]

I added a comment to the commit before landing.

[mcollina]

I think it would be breakage without any reason, and it would not have landed if we were aware that it would break CITGM. Moreover, the change was introduced to fix #15087, which is definitely not about the prototype chain.

@ljharb if you want to pursue this specific change, can you send a different PR so it can be discussed separately?

[apapirovski]

Landed in 38ee25e

[ljharb]

@mcollina totally understand that the change was unintentional and breaking, so of course it should be reverted ASAP.

Philosophically tho, im not sure why it’s desired. Before i invest time in pursuing a change, I’d love to understand why anyone thinks iterating the prototype is a good idea. @addaleax, can you (or anyone else if they share the opinion) elaborate on why?

[addaleax]

@ljharb I’m not sure, but it does feel more natural to me – in my head, the child’s process.env should match what the parent passed in for env as closely as possible. (But don’t worry, I wouldn’t fight over this if somebody opened a deliberate semver-major PR to change it again.)

[joyeecheung]

We should look into why CITGM (or the modules it uses) relies on the prototype properties passed as env though.

[mcollina]

It’d be trivial to change: https://github.com/nodejs/citgm/blob/master/lib/create-options.js.

[jasnell]

In general, I'd be fine with limiting it to own properties only in a semver-major.

[apapirovski]

What's not trivial is that there's an absolute ton of user-land code that relies on this behaviour:

https://github.com/search?q=object.create%28process.env%29&type=Code&utf8=%E2%9C%93
https://github.com/search?utf8=%E2%9C%93&q=%22object.create%28process.env%29%22&type=Code (this is a bit more precise — less unrelated code — but misses some of the instances that the above search finds)

It appears to be a VERY common pattern.

[ljharb]

That’s indeed a strong argument that it’s not worth the change, even if i think that “own keys only” is the correct/better conceptual model.

[apapirovski]

This reverts part of the behaviour introduced in a recent PR, and updates the test. Without this change, CitGM and other packages are broken. I would like to have this fast-tracked (like, land within an hour or less) because as things are, CitGM is completely broken.

Also, I would like to propose, that going forward all changes that are labeled as semver-minor/major/patch MUST have a CitGM run to land. If any issues are found, collaborators & TSC should decide whether those are blocking or not. In either case, an effort should be made to fix the impacted modules before the PR lands — or at the very least in cases where those modules are integral to the Node.js core project.

Refs: 85739b6
Fixes: nodejs/citgm#536

Checklist

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

Affected core subsystem(s)

child_process, test

[evanlucas]

Should we not revert this entirely?

[apapirovski]

Should we not revert this entirely?

We don't need to. Just the prototype portion is excluded. To be honest, IMO that portion of the change kind of snuck through because the documented part is only re: undefined values.

[apapirovski]

CI: https://ci.nodejs.org/job/node-test-pull-request/12586/
CitGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1206/

(Does CitGM use the newly built node version to run itself? If not, that might not work so well...)

[joyeecheung]

@apapirovski I think it does because the previous citgm errors seemed to be caused by citgm itself, not the modules that it was testing.

[richardlau]

Note that some of the modules tested by CitGM will fail on master: nodejs/citgm#517

[addaleax]

To be clear, my approval is not just +1 to landing this but also +1 to considering this the right behaviour.

[ljharb]

Why is this conceptually the right behavior? By far, the language conventions when reflecting on objects are for only including own enumerable properties - eg Object.keys/values/entries/assign and object spread.

[mcollina]

LGTM with a nit.

[mcollina]

I would add a comment that flag that states that copying the prototype values is intentional.

[apapirovski]

I added a comment to the commit before landing.

[mcollina]

I think it would be breakage without any reason, and it would not have landed if we were aware that it would break CITGM. Moreover, the change was introduced to fix #15087, which is definitely not about the prototype chain.

@ljharb if you want to pursue this specific change, can you send a different PR so it can be discussed separately?

[apapirovski]

Landed in 38ee25e

[ljharb]

@mcollina totally understand that the change was unintentional and breaking, so of course it should be reverted ASAP.

Philosophically tho, im not sure why it’s desired. Before i invest time in pursuing a change, I’d love to understand why anyone thinks iterating the prototype is a good idea. @addaleax, can you (or anyone else if they share the opinion) elaborate on why?

[addaleax]

@ljharb I’m not sure, but it does feel more natural to me – in my head, the child’s process.env should match what the parent passed in for env as closely as possible. (But don’t worry, I wouldn’t fight over this if somebody opened a deliberate semver-major PR to change it again.)

[joyeecheung]

We should look into why CITGM (or the modules it uses) relies on the prototype properties passed as env though.

[mcollina]

It’d be trivial to change: https://github.com/nodejs/citgm/blob/master/lib/create-options.js.

[jasnell]

In general, I'd be fine with limiting it to own properties only in a semver-major.

[apapirovski]

What's not trivial is that there's an absolute ton of user-land code that relies on this behaviour:

https://github.com/search?q=object.create%28process.env%29&type=Code&utf8=%E2%9C%93
https://github.com/search?utf8=%E2%9C%93&q=%22object.create%28process.env%29%22&type=Code (this is a bit more precise — less unrelated code — but misses some of the instances that the above search finds)

It appears to be a VERY common pattern.

[ljharb]

That’s indeed a strong argument that it’s not worth the change, even if i think that “own keys only” is the correct/better conceptual model.

[apapirovski]

This reverts part of the behaviour introduced in a recent PR, and updates the test. Without this change, CitGM and other packages are broken. I would like to have this fast-tracked (like, land within an hour or less) because as things are, CitGM is completely broken.

Also, I would like to propose, that going forward all changes that are labeled as semver-minor/major/patch MUST have a CitGM run to land. If any issues are found, collaborators & TSC should decide whether those are blocking or not. In either case, an effort should be made to fix the impacted modules before the PR lands — or at the very least in cases where those modules are integral to the Node.js core project.

Refs: 85739b6
Fixes: nodejs/citgm#536

Checklist

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

Affected core subsystem(s)

child_process, test

[evanlucas]

Should we not revert this entirely?

[apapirovski]

Should we not revert this entirely?

We don't need to. Just the prototype portion is excluded. To be honest, IMO that portion of the change kind of snuck through because the documented part is only re: undefined values.

[apapirovski]

CI: https://ci.nodejs.org/job/node-test-pull-request/12586/
CitGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1206/

(Does CitGM use the newly built node version to run itself? If not, that might not work so well...)

[joyeecheung]

@apapirovski I think it does because the previous citgm errors seemed to be caused by citgm itself, not the modules that it was testing.

[richardlau]

Note that some of the modules tested by CitGM will fail on master: nodejs/citgm#517

[addaleax]

To be clear, my approval is not just +1 to landing this but also +1 to considering this the right behaviour.

[ljharb]

Why is this conceptually the right behavior? By far, the language conventions when reflecting on objects are for only including own enumerable properties - eg Object.keys/values/entries/assign and object spread.

[mcollina]

LGTM with a nit.

[mcollina]

I would add a comment that flag that states that copying the prototype values is intentional.

[apapirovski]

I added a comment to the commit before landing.

[mcollina]

I think it would be breakage without any reason, and it would not have landed if we were aware that it would break CITGM. Moreover, the change was introduced to fix #15087, which is definitely not about the prototype chain.

@ljharb if you want to pursue this specific change, can you send a different PR so it can be discussed separately?

[apapirovski]

Landed in 38ee25e

[ljharb]

@mcollina totally understand that the change was unintentional and breaking, so of course it should be reverted ASAP.

Philosophically tho, im not sure why it’s desired. Before i invest time in pursuing a change, I’d love to understand why anyone thinks iterating the prototype is a good idea. @addaleax, can you (or anyone else if they share the opinion) elaborate on why?

[addaleax]

@ljharb I’m not sure, but it does feel more natural to me – in my head, the child’s process.env should match what the parent passed in for env as closely as possible. (But don’t worry, I wouldn’t fight over this if somebody opened a deliberate semver-major PR to change it again.)

[joyeecheung]

We should look into why CITGM (or the modules it uses) relies on the prototype properties passed as env though.

[mcollina]

It’d be trivial to change: https://github.com/nodejs/citgm/blob/master/lib/create-options.js.

[jasnell]

In general, I'd be fine with limiting it to own properties only in a semver-major.

[apapirovski]

What's not trivial is that there's an absolute ton of user-land code that relies on this behaviour:

https://github.com/search?q=object.create%28process.env%29&type=Code&utf8=%E2%9C%93
https://github.com/search?utf8=%E2%9C%93&q=%22object.create%28process.env%29%22&type=Code (this is a bit more precise — less unrelated code — but misses some of the instances that the above search finds)

It appears to be a VERY common pattern.

[ljharb]

That’s indeed a strong argument that it’s not worth the change, even if i think that “own keys only” is the correct/better conceptual model.

[apapirovski]

This reverts part of the behaviour introduced in a recent PR, and updates the test. Without this change, CitGM and other packages are broken. I would like to have this fast-tracked (like, land within an hour or less) because as things are, CitGM is completely broken.

Also, I would like to propose, that going forward all changes that are labeled as semver-minor/major/patch MUST have a CitGM run to land. If any issues are found, collaborators & TSC should decide whether those are blocking or not. In either case, an effort should be made to fix the impacted modules before the PR lands — or at the very least in cases where those modules are integral to the Node.js core project.

Refs: 85739b6
Fixes: nodejs/citgm#536

Checklist

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

Affected core subsystem(s)

child_process, test

[evanlucas]

Should we not revert this entirely?

[apapirovski]

Should we not revert this entirely?

We don't need to. Just the prototype portion is excluded. To be honest, IMO that portion of the change kind of snuck through because the documented part is only re: undefined values.

[apapirovski]

CI: https://ci.nodejs.org/job/node-test-pull-request/12586/
CitGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1206/

(Does CitGM use the newly built node version to run itself? If not, that might not work so well...)

[joyeecheung]

@apapirovski I think it does because the previous citgm errors seemed to be caused by citgm itself, not the modules that it was testing.

[richardlau]

Note that some of the modules tested by CitGM will fail on master: nodejs/citgm#517

[addaleax]

To be clear, my approval is not just +1 to landing this but also +1 to considering this the right behaviour.

[ljharb]

Why is this conceptually the right behavior? By far, the language conventions when reflecting on objects are for only including own enumerable properties - eg Object.keys/values/entries/assign and object spread.

[mcollina]

LGTM with a nit.

[mcollina]

I would add a comment that flag that states that copying the prototype values is intentional.

[apapirovski]

I added a comment to the commit before landing.

[mcollina]

I think it would be breakage without any reason, and it would not have landed if we were aware that it would break CITGM. Moreover, the change was introduced to fix #15087, which is definitely not about the prototype chain.

@ljharb if you want to pursue this specific change, can you send a different PR so it can be discussed separately?

[apapirovski]

Landed in 38ee25e

[ljharb]

@mcollina totally understand that the change was unintentional and breaking, so of course it should be reverted ASAP.

Philosophically tho, im not sure why it’s desired. Before i invest time in pursuing a change, I’d love to understand why anyone thinks iterating the prototype is a good idea. @addaleax, can you (or anyone else if they share the opinion) elaborate on why?

[addaleax]

@ljharb I’m not sure, but it does feel more natural to me – in my head, the child’s process.env should match what the parent passed in for env as closely as possible. (But don’t worry, I wouldn’t fight over this if somebody opened a deliberate semver-major PR to change it again.)

[joyeecheung]

We should look into why CITGM (or the modules it uses) relies on the prototype properties passed as env though.

[mcollina]

It’d be trivial to change: https://github.com/nodejs/citgm/blob/master/lib/create-options.js.

[jasnell]

In general, I'd be fine with limiting it to own properties only in a semver-major.

[apapirovski]

What's not trivial is that there's an absolute ton of user-land code that relies on this behaviour:

https://github.com/search?q=object.create%28process.env%29&type=Code&utf8=%E2%9C%93
https://github.com/search?utf8=%E2%9C%93&q=%22object.create%28process.env%29%22&type=Code (this is a bit more precise — less unrelated code — but misses some of the instances that the above search finds)

It appears to be a VERY common pattern.

[ljharb]

That’s indeed a strong argument that it’s not worth the change, even if i think that “own keys only” is the correct/better conceptual model.

[apapirovski]

This reverts part of the behaviour introduced in a recent PR, and updates the test. Without this change, CitGM and other packages are broken. I would like to have this fast-tracked (like, land within an hour or less) because as things are, CitGM is completely broken.

Also, I would like to propose, that going forward all changes that are labeled as semver-minor/major/patch MUST have a CitGM run to land. If any issues are found, collaborators & TSC should decide whether those are blocking or not. In either case, an effort should be made to fix the impacted modules before the PR lands — or at the very least in cases where those modules are integral to the Node.js core project.

Refs: 85739b6
Fixes: nodejs/citgm#536

Checklist

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

Affected core subsystem(s)

child_process, test

[evanlucas]

Should we not revert this entirely?

[apapirovski]

Should we not revert this entirely?

We don't need to. Just the prototype portion is excluded. To be honest, IMO that portion of the change kind of snuck through because the documented part is only re: undefined values.

[apapirovski]

CI: https://ci.nodejs.org/job/node-test-pull-request/12586/
CitGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1206/

(Does CitGM use the newly built node version to run itself? If not, that might not work so well...)

[joyeecheung]

@apapirovski I think it does because the previous citgm errors seemed to be caused by citgm itself, not the modules that it was testing.

[richardlau]

Note that some of the modules tested by CitGM will fail on master: nodejs/citgm#517

[addaleax]

To be clear, my approval is not just +1 to landing this but also +1 to considering this the right behaviour.

[ljharb]

Why is this conceptually the right behavior? By far, the language conventions when reflecting on objects are for only including own enumerable properties - eg Object.keys/values/entries/assign and object spread.

[mcollina]

LGTM with a nit.

[mcollina]

I would add a comment that flag that states that copying the prototype values is intentional.

[apapirovski]

I added a comment to the commit before landing.

[mcollina]

I think it would be breakage without any reason, and it would not have landed if we were aware that it would break CITGM. Moreover, the change was introduced to fix #15087, which is definitely not about the prototype chain.

@ljharb if you want to pursue this specific change, can you send a different PR so it can be discussed separately?

[apapirovski]

Landed in 38ee25e

[ljharb]

@mcollina totally understand that the change was unintentional and breaking, so of course it should be reverted ASAP.

Philosophically tho, im not sure why it’s desired. Before i invest time in pursuing a change, I’d love to understand why anyone thinks iterating the prototype is a good idea. @addaleax, can you (or anyone else if they share the opinion) elaborate on why?

[addaleax]

@ljharb I’m not sure, but it does feel more natural to me – in my head, the child’s process.env should match what the parent passed in for env as closely as possible. (But don’t worry, I wouldn’t fight over this if somebody opened a deliberate semver-major PR to change it again.)

[joyeecheung]

We should look into why CITGM (or the modules it uses) relies on the prototype properties passed as env though.

[mcollina]

It’d be trivial to change: https://github.com/nodejs/citgm/blob/master/lib/create-options.js.

[jasnell]

In general, I'd be fine with limiting it to own properties only in a semver-major.

[apapirovski]

What's not trivial is that there's an absolute ton of user-land code that relies on this behaviour:

https://github.com/search?q=object.create%28process.env%29&type=Code&utf8=%E2%9C%93
https://github.com/search?utf8=%E2%9C%93&q=%22object.create%28process.env%29%22&type=Code (this is a bit more precise — less unrelated code — but misses some of the instances that the above search finds)

It appears to be a VERY common pattern.

[ljharb]

That’s indeed a strong argument that it’s not worth the change, even if i think that “own keys only” is the correct/better conceptual model.

[apapirovski]

This reverts part of the behaviour introduced in a recent PR, and updates the test. Without this change, CitGM and other packages are broken. I would like to have this fast-tracked (like, land within an hour or less) because as things are, CitGM is completely broken.

Also, I would like to propose, that going forward all changes that are labeled as semver-minor/major/patch MUST have a CitGM run to land. If any issues are found, collaborators & TSC should decide whether those are blocking or not. In either case, an effort should be made to fix the impacted modules before the PR lands — or at the very least in cases where those modules are integral to the Node.js core project.

Refs: 85739b6
Fixes: nodejs/citgm#536

Checklist

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

Affected core subsystem(s)

child_process, test

[evanlucas]

Should we not revert this entirely?

[apapirovski]

Should we not revert this entirely?

We don't need to. Just the prototype portion is excluded. To be honest, IMO that portion of the change kind of snuck through because the documented part is only re: undefined values.

[apapirovski]

CI: https://ci.nodejs.org/job/node-test-pull-request/12586/
CitGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1206/

(Does CitGM use the newly built node version to run itself? If not, that might not work so well...)

[joyeecheung]

@apapirovski I think it does because the previous citgm errors seemed to be caused by citgm itself, not the modules that it was testing.

[richardlau]

Note that some of the modules tested by CitGM will fail on master: nodejs/citgm#517

[addaleax]

To be clear, my approval is not just +1 to landing this but also +1 to considering this the right behaviour.

[ljharb]

Why is this conceptually the right behavior? By far, the language conventions when reflecting on objects are for only including own enumerable properties - eg Object.keys/values/entries/assign and object spread.

[mcollina]

LGTM with a nit.

[mcollina]

I would add a comment that flag that states that copying the prototype values is intentional.

[apapirovski]

I added a comment to the commit before landing.

[mcollina]

I think it would be breakage without any reason, and it would not have landed if we were aware that it would break CITGM. Moreover, the change was introduced to fix #15087, which is definitely not about the prototype chain.

@ljharb if you want to pursue this specific change, can you send a different PR so it can be discussed separately?

[apapirovski]

Landed in 38ee25e

[ljharb]

@mcollina totally understand that the change was unintentional and breaking, so of course it should be reverted ASAP.

Philosophically tho, im not sure why it’s desired. Before i invest time in pursuing a change, I’d love to understand why anyone thinks iterating the prototype is a good idea. @addaleax, can you (or anyone else if they share the opinion) elaborate on why?

[addaleax]

@ljharb I’m not sure, but it does feel more natural to me – in my head, the child’s process.env should match what the parent passed in for env as closely as possible. (But don’t worry, I wouldn’t fight over this if somebody opened a deliberate semver-major PR to change it again.)

[joyeecheung]

We should look into why CITGM (or the modules it uses) relies on the prototype properties passed as env though.

[mcollina]

It’d be trivial to change: https://github.com/nodejs/citgm/blob/master/lib/create-options.js.

[jasnell]

In general, I'd be fine with limiting it to own properties only in a semver-major.

[apapirovski]

What's not trivial is that there's an absolute ton of user-land code that relies on this behaviour:

https://github.com/search?q=object.create%28process.env%29&type=Code&utf8=%E2%9C%93
https://github.com/search?utf8=%E2%9C%93&q=%22object.create%28process.env%29%22&type=Code (this is a bit more precise — less unrelated code — but misses some of the instances that the above search finds)

It appears to be a VERY common pattern.

[ljharb]

That’s indeed a strong argument that it’s not worth the change, even if i think that “own keys only” is the correct/better conceptual model.

[apapirovski]

This reverts part of the behaviour introduced in a recent PR, and updates the test. Without this change, CitGM and other packages are broken. I would like to have this fast-tracked (like, land within an hour or less) because as things are, CitGM is completely broken.

Also, I would like to propose, that going forward all changes that are labeled as semver-minor/major/patch MUST have a CitGM run to land. If any issues are found, collaborators & TSC should decide whether those are blocking or not. In either case, an effort should be made to fix the impacted modules before the PR lands — or at the very least in cases where those modules are integral to the Node.js core project.

Refs: 85739b6
Fixes: nodejs/citgm#536

Checklist

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

Affected core subsystem(s)

child_process, test

[evanlucas]

Should we not revert this entirely?

[apapirovski]

Should we not revert this entirely?

We don't need to. Just the prototype portion is excluded. To be honest, IMO that portion of the change kind of snuck through because the documented part is only re: undefined values.

[apapirovski]

CI: https://ci.nodejs.org/job/node-test-pull-request/12586/
CitGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1206/

(Does CitGM use the newly built node version to run itself? If not, that might not work so well...)

[joyeecheung]

@apapirovski I think it does because the previous citgm errors seemed to be caused by citgm itself, not the modules that it was testing.

[richardlau]

Note that some of the modules tested by CitGM will fail on master: nodejs/citgm#517

[addaleax]

To be clear, my approval is not just +1 to landing this but also +1 to considering this the right behaviour.

[ljharb]

Why is this conceptually the right behavior? By far, the language conventions when reflecting on objects are for only including own enumerable properties - eg Object.keys/values/entries/assign and object spread.

[mcollina]

LGTM with a nit.

[mcollina]

I would add a comment that flag that states that copying the prototype values is intentional.

[apapirovski]

I added a comment to the commit before landing.

[mcollina]

I think it would be breakage without any reason, and it would not have landed if we were aware that it would break CITGM. Moreover, the change was introduced to fix #15087, which is definitely not about the prototype chain.

@ljharb if you want to pursue this specific change, can you send a different PR so it can be discussed separately?

[apapirovski]

Landed in 38ee25e

[ljharb]

@mcollina totally understand that the change was unintentional and breaking, so of course it should be reverted ASAP.

Philosophically tho, im not sure why it’s desired. Before i invest time in pursuing a change, I’d love to understand why anyone thinks iterating the prototype is a good idea. @addaleax, can you (or anyone else if they share the opinion) elaborate on why?

[addaleax]

@ljharb I’m not sure, but it does feel more natural to me – in my head, the child’s process.env should match what the parent passed in for env as closely as possible. (But don’t worry, I wouldn’t fight over this if somebody opened a deliberate semver-major PR to change it again.)

[joyeecheung]

We should look into why CITGM (or the modules it uses) relies on the prototype properties passed as env though.

[mcollina]

It’d be trivial to change: https://github.com/nodejs/citgm/blob/master/lib/create-options.js.

[jasnell]

In general, I'd be fine with limiting it to own properties only in a semver-major.

[apapirovski]

What's not trivial is that there's an absolute ton of user-land code that relies on this behaviour:

https://github.com/search?q=object.create%28process.env%29&type=Code&utf8=%E2%9C%93
https://github.com/search?utf8=%E2%9C%93&q=%22object.create%28process.env%29%22&type=Code (this is a bit more precise — less unrelated code — but misses some of the instances that the above search finds)

It appears to be a VERY common pattern.

[ljharb]

That’s indeed a strong argument that it’s not worth the change, even if i think that “own keys only” is the correct/better conceptual model.