Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lib: deep-copy process.config during configure #2368

Merged
merged 2 commits into from May 29, 2021

Conversation

DeeDeeG
Copy link
Contributor

@DeeDeeG DeeDeeG commented Apr 11, 2021

Checklist
  • npm install && npm test passes
  • tests are included
  • documentation is changed or added
  • commit message follows commit guidelines
Description of change

In lib/configure.js, when creating the local config object as a copy of process.config, use JSON.parse(JSON.stringify(process.config) rather than Object.assign({}, process.config).

Makes it so we won't modify properties of child objects of the original process.config. (Modifying process.config or its children is deprecated as of Node 16.)

Background

Testing with the latest Node v16 nightly build revealed that the deprecation warning from nodejs/node#36902 was still showing with the latest node-gyp v8.0.0 during the config or rebuild commands.

// ...
gyp info find Python using Python version 3.9.0 found at "/usr/local/bin/python3"
(node:21174) [DEP0150] DeprecationWarning: Setting process.config is deprecated. In the future the property will be read-only.
(Use `node --trace-deprecation ...` to show where the warning was created)
gyp info spawn /usr/local/bin/python3
// ...

With the --trace-deprecation flag:

// ...
gyp info find Python using Python version 3.9.0 found at "/usr/local/bin/python3"
(node:21184) [DEP0150] DeprecationWarning: Setting process.config is deprecated. In the future the property will be read-only.
    at Object.maybeWarn (node:internal/bootstrap/node:79:15)
    at Object.set (node:internal/bootstrap/node:103:10)
    at createConfigFile (/Users/[user]/node-gyp/lib/configure.js:117:21)
    at /Users/[user]/node-gyp/lib/configure.js:84:9
    at FSReqCallback.oncomplete (node:fs:183:23)
gyp info spawn /usr/local/bin/python3
// ...

In Node v16, the process.config object has a proxy function which warns once if it, or any of its properties, or any of its child objects' properties, are modified. And it turns out that Object.assign() does not deep copy any nested objects of the object you assign from. Nested objects are copied by reference rather than by value. So by modifying (for example) our local config.defaults.cflags array, we are also modifying process.config.target_defaults.cflags.

See the discussion at the bottom of #2322 for more details.

Makes it entirely sure that we won't modify the original process.config.
(Modifying process.config or its children is deprecated as of Node 16.)
Avoids errors in JSON.parse if process.config has been deleted
@gengjiawen gengjiawen merged commit 5f1a06c into nodejs:master May 29, 2021
@DeeDeeG
Copy link
Contributor Author

DeeDeeG commented May 30, 2021

Hi again, thanks for merging.

I just wanted to mention: Apparently JSON.stringify can error out if you ask it to stringify an object with a BigInt or a circular reference in it.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/stringify#exceptions

I don't think people should be putting either of those things in their process.config object. And altering process.config is of course deprecated as of Node 16, but it is still possible to do.

So, for dealing with this unlikely/bizarre case, we could consider handling the error somehow or using a purpose-built library for deep-copying objects. I don't really think that's worth it, but I thought I would raise the issue. (I forgot to mention it earlier.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants