url: move process.binding('url') to internalBinding#22204
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot added c++ 
starkwang commented Aug 9, 2018
maclover7 commented Aug 9, 2018
Hmm, here's looks like edit: is also failing on Debian 8 x64, Debian 8 x86, Debian 9, and Fedora 26 |
devsnek added the semver-major 
jasnell commented Aug 9, 2018
The failure in gulp makes sense given it's dependency on |
jasnell left a comment
jasnell left a comment There was a problem hiding this comment.
This one should not land yet because of the natives failure. It is unfortunate, but we should hold off a bit on process.binding() changes that impact the fs module directly.
devsnek commented Aug 12, 2018
jasnell commented Aug 12, 2018
Potentially yes. We should still be careful. |
jdalton left a comment
jdalton left a comment There was a problem hiding this comment.
Holding removals until migration is ironed out
jasnell commented Aug 15, 2018
@starkwang ... when you get a moment, can you rebase this on master then apply the following patch that will be required for this to proceed: Author: James M Snell <[email protected]> Date: Tue Aug 14 19:36:25 2018 -0700 [Squash] add `process.binding('url')` to fallthrough whitelist diff --git a/lib/internal/bootstrap/node.js b/lib/internal/bootstrap/node.js index 08daeb1915..ffae6e4cd9 100644 --- a/lib/internal/bootstrap/node.js+++ b/lib/internal/bootstrap/node.js@@ -327,7 +327,7 @@ // that are whitelisted for access via process.binding()... this is used // to provide a transition path for modules that are being moved over to // internalBinding. - const internalBindingWhitelist = new SafeSet(['uv']);+ const internalBindingWhitelist = new SafeSet(['uv', 'url']); process.binding = function binding(name){return internalBindingWhitelist.has(name) ? internalBinding(name) : diff --git a/test/parallel/test-process-binding-internalbinding-whitelist.js b/test/parallel/test-process-binding-internalbinding-whitelist.js index ece967a0b7..cc0119917d 100644 --- a/test/parallel/test-process-binding-internalbinding-whitelist.js+++ b/test/parallel/test-process-binding-internalbinding-whitelist.js@@ -7,3 +7,4 @@ const assert = require('assert'); // Assert that whitelisted internalBinding modules are accessible via // process.binding(). assert(process.binding('uv')); +assert(process.binding('url')); |
starkwangforce-pushed the url-internal-binding branch from a820ce5 to a27fc75Comparestarkwang commented Aug 15, 2018
The branch has been rebased on master and applied the patch. |
starkwang commented Aug 15, 2018
Umm... I can't open a new CI for this because it is blocked by nodejs/build#1442. |
jasnell commented Aug 15, 2018
Yep, no worries... once the release happens the CI will be unrestricted again. |
starkwang commented Aug 16, 2018
starkwangforce-pushed the url-internal-binding branch from a27fc75 to 6b79af7Comparestarkwang commented Aug 20, 2018
Rebased branch to resolve conflicts. |
jasnell commented Aug 20, 2018
jasnell commented Aug 20, 2018
Ugh, another unrelated CI failure... resuming: https://ci.nodejs.org/job/node-test-pull-request/16621/ |
BridgeAR commented Sep 5, 2018
This needs a rebase. |
starkwangforce-pushed the url-internal-binding branch from 6b79af7 to 1f62167Comparestarkwang commented Sep 6, 2018
Rebased and opened a new CI: https://ci.nodejs.org/job/node-test-pull-request/17058/ |
BridgeAR added the author ready starkwang commented Sep 7, 2018
Landed in e917a23. |
PR-URL: #22204 Refs: #22160 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: John-David Dalton <[email protected]>
10 participants
Refs: #22160
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes