process: freeze process.features

[joyeecheung]

Refs: #25343

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

[nodejs-github-bot]

Sadly, an error occurred when I tried to trigger a build. :(

[addaleax]

Hm, any particular reason for doing so? It seems like this might get in the way of mocking for testing, and I don’t really see how one would modify this object accidentally (= not on purpose)

[jasnell]

Yeah, I'm not sure there's a really good reason to do this

[joyeecheung]

@addaleax See #25343 (comment)
@silverwind

Not that I feel very strongly about this PR in particular, but from my experience mocking Node.js to test anything is usually a bad idea - that means the test can be easily broken by upstream changes that do not affect the functionality that you are actually testing, which would've been impossible if the code is written in some other stricter language or more robust platform. Not just mocking the platform - even mocking your own code can lead to very fragile tests.

[devsnek]

also consider that a library may polyfill functionality and wish to therefore modify process.features.

[silverwind]

My point about freezing is that a malicious library/actor could modify values and break software relying on process.features that way. Thought I do agree that "polyfills" (I think we call them monkey patches outside browsers) may present a valid use case.

Mocking for tests is not a valid reason to not freeze imho. We could instead just introduce a internal flag that is set during test runs which would not freeze it instead.

[addaleax]

Mocking for tests is not a valid reason to not freeze imho. We could instead just introduce a internal flag that is set during test runs which would not freeze it instead.

To clarify, when I talked about tests I was talking about other people’s tests, not ours.

But more generally, I dislike the idea of giving JS objects special behaviour unless there’s a very solid reason to do so; JS objects are writable by default, and each exception is going to break somebody’s expectations (aka principle of least surprise).

[BridgeAR]

What should be done here?

[joyeecheung]

I agree that allowing polyfills to modify this is a valid use case, so I will close this.