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

Merged
gengjiawen merged 2 commits into
nodejs:masterfrom
DeeDeeG:json_stringify-process_config
May 29, 2021
Merged

lib: deep-copy process.config during configure#2368
gengjiawen merged 2 commits into
nodejs:masterfrom
DeeDeeG:json_stringify-process_config

Conversation

@DeeDeeG

@DeeDeeGDeeDeeG commented Apr 11, 2021•
edited
Loading

Copy link
Copy Markdown
Contributor
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.

@DeeDeeG
lib: deep-copy process.config during configure
fdc30c6
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.)
@DeeDeeGDeeDeeG mentioned this pull request Apr 11, 2021
Closed
@DeeDeeG
lib: set empty object if no process.config
eb24896
Avoids errors in JSON.parse if process.config has been deleted
@gengjiawen
gengjiawen merged commit 5f1a06c into nodejs:masterMay 29, 2021
@DeeDeeG

DeeDeeG commented May 30, 2021•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

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 freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gengjiawengengjiawengengjiawen approved these changes
@richardlaurichardlaurichardlau approved these changes
+1 more reviewer
@imatlopezimatlopezimatlopez approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@DeeDeeG@gengjiawen@richardlau@imatlopez
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
lib: deep-copy process.config during configure - #2368
Merged
gengjiawen merged 2 commits into
nodejs:masterfrom 
DeeDeeG:json_stringify-process_config
May 29, 2021
Merged
lib: deep-copy process.config during configure#2368
gengjiawen merged 2 commits into
nodejs:masterfrom 
DeeDeeG:json_stringify-process_config
Conversation
@DeeDeeG
@DeeDeeGDeeDeeG
commented
Apr 11, 2021•

edited

Loading

Copy link


Copy Markdown

Contributor
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.
@DeeDeeG
lib: deep-copy process.config during configure
fdc30c6
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.)
@DeeDeeGDeeDeeG
mentioned this pull request
Apr 11, 2021

Closed

@DeeDeeG
lib: set empty object if no process.config
eb24896
Avoids errors in JSON.parse if process.config has been deleted
@gengjiawen
gengjiawen
merged commit 5f1a06c
into
nodejs:masterMay 29, 2021
@DeeDeeG
DeeDeeG
commented
May 30, 2021•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@gengjiawengengjiawengengjiawen approved these changes
@richardlaurichardlaurichardlau approved these changes

+1 more reviewer

@imatlopezimatlopezimatlopez approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@DeeDeeG@gengjiawen@richardlau@imatlopez

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
lib: deep-copy process.config during configure - #2368
Merged
gengjiawen merged 2 commits into
nodejs:masterfrom 
DeeDeeG:json_stringify-process_config
May 29, 2021
Merged
lib: deep-copy process.config during configure#2368
gengjiawen merged 2 commits into
nodejs:masterfrom 
DeeDeeG:json_stringify-process_config
Conversation
@DeeDeeG
@DeeDeeGDeeDeeG
commented
Apr 11, 2021•

edited

Loading

Copy link


Copy Markdown

Contributor
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.
@DeeDeeG
lib: deep-copy process.config during configure
fdc30c6
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.)
@DeeDeeGDeeDeeG
mentioned this pull request
Apr 11, 2021

Closed

@DeeDeeG
lib: set empty object if no process.config
eb24896
Avoids errors in JSON.parse if process.config has been deleted
@gengjiawen
gengjiawen
merged commit 5f1a06c
into
nodejs:masterMay 29, 2021
@DeeDeeG
DeeDeeG
commented
May 30, 2021•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@gengjiawengengjiawengengjiawen approved these changes
@richardlaurichardlaurichardlau approved these changes

+1 more reviewer

@imatlopezimatlopezimatlopez approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@DeeDeeG@gengjiawen@richardlau@imatlopez

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
lib: deep-copy process.config during configure - #2368
Merged
gengjiawen merged 2 commits into
nodejs:masterfrom 
DeeDeeG:json_stringify-process_config
May 29, 2021
Merged
lib: deep-copy process.config during configure#2368
gengjiawen merged 2 commits into
nodejs:masterfrom 
DeeDeeG:json_stringify-process_config
Conversation
@DeeDeeG
@DeeDeeGDeeDeeG
commented
Apr 11, 2021•

edited

Loading

Copy link


Copy Markdown

Contributor
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.
@DeeDeeG
lib: deep-copy process.config during configure
fdc30c6
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.)
@DeeDeeGDeeDeeG
mentioned this pull request
Apr 11, 2021

Closed

@DeeDeeG
lib: set empty object if no process.config
eb24896
Avoids errors in JSON.parse if process.config has been deleted
@gengjiawen
gengjiawen
merged commit 5f1a06c
into
nodejs:masterMay 29, 2021
@DeeDeeG
DeeDeeG
commented
May 30, 2021•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@gengjiawengengjiawengengjiawen approved these changes
@richardlaurichardlaurichardlau approved these changes

+1 more reviewer

@imatlopezimatlopezimatlopez approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@DeeDeeG@gengjiawen@richardlau@imatlopez

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
lib: deep-copy process.config during configure - #2368
Merged
gengjiawen merged 2 commits into
nodejs:masterfrom 
DeeDeeG:json_stringify-process_config
May 29, 2021
Merged
lib: deep-copy process.config during configure#2368
gengjiawen merged 2 commits into
nodejs:masterfrom 
DeeDeeG:json_stringify-process_config
Conversation
@DeeDeeG
@DeeDeeGDeeDeeG
commented
Apr 11, 2021•

edited

Loading

Copy link


Copy Markdown

Contributor
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.
@DeeDeeG
lib: deep-copy process.config during configure
fdc30c6
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.)
@DeeDeeGDeeDeeG
mentioned this pull request
Apr 11, 2021

Closed

@DeeDeeG
lib: set empty object if no process.config
eb24896
Avoids errors in JSON.parse if process.config has been deleted
@gengjiawen
gengjiawen
merged commit 5f1a06c
into
nodejs:masterMay 29, 2021
@DeeDeeG
DeeDeeG
commented
May 30, 2021•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@gengjiawengengjiawengengjiawen approved these changes
@richardlaurichardlaurichardlau approved these changes

+1 more reviewer

@imatlopezimatlopezimatlopez approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@DeeDeeG@gengjiawen@richardlau@imatlopez

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
lib: deep-copy process.config during configure - #2368
Merged
gengjiawen merged 2 commits into
nodejs:masterfrom 
DeeDeeG:json_stringify-process_config
May 29, 2021
Merged
lib: deep-copy process.config during configure#2368
gengjiawen merged 2 commits into
nodejs:masterfrom 
DeeDeeG:json_stringify-process_config
Conversation
@DeeDeeG
@DeeDeeGDeeDeeG
commented
Apr 11, 2021•

edited

Loading

Copy link


Copy Markdown

Contributor
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.
@DeeDeeG
lib: deep-copy process.config during configure
fdc30c6
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.)
@DeeDeeGDeeDeeG
mentioned this pull request
Apr 11, 2021

Closed

@DeeDeeG
lib: set empty object if no process.config
eb24896
Avoids errors in JSON.parse if process.config has been deleted
@gengjiawen
gengjiawen
merged commit 5f1a06c
into
nodejs:masterMay 29, 2021
@DeeDeeG
DeeDeeG
commented
May 30, 2021•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@gengjiawengengjiawengengjiawen approved these changes
@richardlaurichardlaurichardlau approved these changes

+1 more reviewer

@imatlopezimatlopezimatlopez approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@DeeDeeG@gengjiawen@richardlau@imatlopez

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
lib: deep-copy process.config during configure - #2368
Merged
gengjiawen merged 2 commits into
nodejs:masterfrom 
DeeDeeG:json_stringify-process_config
May 29, 2021
Merged
lib: deep-copy process.config during configure#2368
gengjiawen merged 2 commits into
nodejs:masterfrom 
DeeDeeG:json_stringify-process_config
Conversation
@DeeDeeG
@DeeDeeGDeeDeeG
commented
Apr 11, 2021•

edited

Loading

Copy link


Copy Markdown

Contributor
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.
@DeeDeeG
lib: deep-copy process.config during configure
fdc30c6
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.)
@DeeDeeGDeeDeeG
mentioned this pull request
Apr 11, 2021

Closed

@DeeDeeG
lib: set empty object if no process.config
eb24896
Avoids errors in JSON.parse if process.config has been deleted
@gengjiawen
gengjiawen
merged commit 5f1a06c
into
nodejs:masterMay 29, 2021
@DeeDeeG
DeeDeeG
commented
May 30, 2021•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@gengjiawengengjiawengengjiawen approved these changes
@richardlaurichardlaurichardlau approved these changes

+1 more reviewer

@imatlopezimatlopezimatlopez approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@DeeDeeG@gengjiawen@richardlau@imatlopez

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
lib: deep-copy process.config during configure - #2368
Merged
gengjiawen merged 2 commits into
nodejs:masterfrom 
DeeDeeG:json_stringify-process_config
May 29, 2021
Merged
lib: deep-copy process.config during configure#2368
gengjiawen merged 2 commits into
nodejs:masterfrom 
DeeDeeG:json_stringify-process_config
Conversation
@DeeDeeG
@DeeDeeGDeeDeeG
commented
Apr 11, 2021•

edited

Loading

Copy link


Copy Markdown

Contributor
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.
@DeeDeeG
lib: deep-copy process.config during configure
fdc30c6
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.)
@DeeDeeGDeeDeeG
mentioned this pull request
Apr 11, 2021

Closed

@DeeDeeG
lib: set empty object if no process.config
eb24896
Avoids errors in JSON.parse if process.config has been deleted
@gengjiawen
gengjiawen
merged commit 5f1a06c
into
nodejs:masterMay 29, 2021
@DeeDeeG
DeeDeeG
commented
May 30, 2021•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@gengjiawengengjiawengengjiawen approved these changes
@richardlaurichardlaurichardlau approved these changes

+1 more reviewer

@imatlopezimatlopezimatlopez approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@DeeDeeG@gengjiawen@richardlau@imatlopez