lib: avoid changing process.config

[targos]

Checklist

• npm install && npm test passes
• commit message follows commit guidelines

Description of change

It is deprecated and will emit a warning in Node.js 16.

Refs: nodejs/node#36902

[targos]

I'll let someone from @nodejs/node-gyp land this.

[rvagg]

@gengjiawen I don't suppose you want to help us get set up with the Actions suggested in #2286 so we can reduce this friction?

[gengjiawen]

@gengjiawen I don't suppose you want to help us get set up with the Actions suggested in #2286 so we can reduce this friction?

Actually I can help with it. I am home for Chinese spring festival, get more free time :)

[DeeDeeG]

Hi, folks.

I was testing node-gyp with NodeJS nightly v16. I noticed node-gyp still changes process.config, and the warning is still emitted on Node 16. I am working on submitting a fix as a PR.

I believe this is because Object.assign() does not deep-copy nested objects under the object being copied. It only creates a reference to the nested object. So we can't modify the new object's nested objects without modifying process.config.

Details of the problem, with annotated code snippet to illustrate (click to expand)

We do the following, which mutates the process.config.target_defaults object nested under process.config:

// lib/configure.js

    var config = Object.assign({}, process.config) // This is not a deep-copy operation!
    var defaults = config.target_defaults // defaults is a pointer to process.config.target_defaults!

    // ... snipped ...

    // don't inherit the "defaults" from node's `process.config` object.
    defaults.cflags = [] // this mutates process.config.target_defaults.cflags !
    defaults.defines = []
    defaults.include_dirs = []
    defaults.libraries = []

I think it's a bit suboptimal to stringify to JSON and parse it back out again, but I am not aware of any other simple way to deep copy an object. deepCopyOfObject = JSON.parse(JSON.stringify(originalObject)).

There are libraries specifically for deep copying objects, but I'm not sure it's worth adding another library just to deep copy process.config.

[rvagg]

I wouldn't be opposed to JSON.parse(JSON.stringify()), it's simple and easy and avoids another dep. process.config is pretty small and simple so it shouldn't cost much. It also wouldn't be hard to write a manual deep copy but JSON might just be easier. Or maybe we can work around it entirely by being a bit smarter about what we pull out?

[DeeDeeG]

Exploring options: we could also do this:

var defaults = Object.assign({}, process.config.target_defaults)
var variables = Object.assign({}, process.config.variables)
var config = { variables, target_defaults: defaults }

(Since process.config is only two layers deep and has not many children, we can directly copy both child objects out.)

I personally find it slightly more confusing to read than the JSON.stringify() approach.

And we have to be confident neither of process.config.target_defaults nor process.config.variables will contain a deeper-nested object in the future, or we could end up mutating process.config again.

[Edit: On second look, that should not be a problem, given the simple and limited ways config is modified in node-gyp's code. We can write anything in a directly-copied object. We just have to avoid mutating a property of a child object of an object we directly copied. If we're going to go any layers deeper than the layer of object we manually copied, then we can only overwrite those deeper objects wholesale, not modify properties in the nested objects.

If the need arises to tinker in deeper layers of objects: As long as we go in and manually copy said deeper objects before modifying their properties, we are fine. Today's process.config is only two objects deep, so the code example above is sufficient.]

microbenchmarks (either fix is about 0.1 - 0.4 milliseconds slower than current master branch) (click to expand)

Original as of this PR: ~0.10 - 0.13 milliseconds (outlier slow runs: ~0.14 - 0.15 milliseconds) (NOTE: deprecated)
Object.assign() fix: ~0.25 - 0.31 milliseconds (outlier slow runs: ~ 0.35 - 0.52 milliseconds)
JSON.stringify() fix: ~0.26 - 0.35 milliseconds (outlier slow runs: ~ 0.46 - 0.56 milliseconds)

These times are from a microbenchmark of just assigning defaults, variables and config with each approach, and doing perf_hooks.performance.now() immediately before and after object assignment. it's not reliably or perceptibly faster than the JSON.stringify() approach. They're both "twice as slow" as doing Object.assign() once, with references instead of deep copying. So yeah, there's a "performance hit"... a performance hit of only about 0.1 to 0.2 milliseconds! (Up to 0.4 milliseconds slower in a few outlier runs.)

I personally lean toward JSON.parse(JSON.stringify) over that. But in general I think these are close to, if not the two optimal solutions for deep copying today's process.config.

[targos]

Note that when I opened this, I knew that the could would still change deep parts of process.config. I only cared about avoiding the warning and I validated it by changing the code directly inside npm/node_modules so I'm surprised that it can still be triggered.

[DeeDeeG]

@targos I have some steps to reproduce. I think these are pretty real-world equivalent:

• Use n to install Node 16 nightly. (command: n nightly). macOS 10.15.7 Catalina.
  • (Effectively: download and extract the Node 16 nightly tarball, ensure its extracted bins dir is on the PATH)
  • Edit: Also tested on Windows 10, same deprecation warning.
• Clone some package repo that builds with node-gyp (I used the superstring package: git clone https://github.com/atom/superstring), npm install its dependencies as normal.
• In the module directory (/Users/[me]/superstring), do npx node-gyp configure.

Edit to add: Neater steps with Node 16 already installed:

• mkdir require_superstring && cd require_superstring && npm init -y && npm install --ignore-scripts superstring
• cd node_modules/superstring && npx node-gyp@8 configure
  • Arg... there is a bug in npm@7.9.0 that makes npx node-gyp@8 run the older node-gyp 7.1.2 bundled with npm. I just reported that to npm's bug tracker... [BUG] npx node-gyp@version runs the copy of node-gyp bundled with npm instead npm/cli#3058. It works with npm v6.

To get the full stack trace for the warning, I did:

• npm install -g node-gyp
• node --trace-deprecation $(command -v node-gyp) configure inside a module directory that build with node-gyp (again, superstring in my case.)

I wonder if something changed in Node in the months since this PR originally happened. But I am seeing the child objects of process.config are frozen too, not just process.config itself.

[targos]

Thanks for the detailed explanation. I just realized that the deprecation was applied deeply using proxies, so yeah we do need to create a deep copy...
I think the JSON stringify/parse approach is fine in this case.