Skip to content

url: move process.binding('url') to internalBinding#22204

Closed
starkwang wants to merge 2 commits intonodejs:masterfrom
starkwang:url-internal-binding
Closed

url: move process.binding('url') to internalBinding#22204
starkwang wants to merge 2 commits intonodejs:masterfrom
starkwang:url-internal-binding

Conversation

@starkwang
Copy link
Contributor

@starkwang starkwang commented Aug 9, 2018 •
edited
Loading

Refs: #22160

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Aug 9, 2018

@starkwang build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/470/pipeline

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Aug 9, 2018
@starkwang
Copy link
Contributor Author

starkwang commented Aug 9, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/16293/

@jasnell jasnell mentioned this pull request Aug 9, 2018
Closed
33 tasks
@maclover7
Copy link
Contributor

maclover7 commented Aug 9, 2018

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1495/

@maclover7
Copy link
Contributor

maclover7 commented Aug 9, 2018 •
edited
Loading

Hmm, here's looks like gulp@3.9.1 is failing on an Ubuntu 16.04 machine (I double checked the git sha and that process.binding('url') threw an error to make sure things were recompiled properly):

 internal/url.js:1441
 setURLConstructor(constructUrl);
 ^
 TypeError: setURLConstructor is not a function
     at internal/url.js:1441:1
     at req_ (/home/iojs/build/workspace/citgm-smoker/nodes/ubuntu1604-64/citgm_tmp/92109405-f794-4108-8120-36349a9efdfa/gulp/node_modules/natives/index.js:137:5)
     at require (/home/iojs/build/workspace/citgm-smoker/nodes/ubuntu1604-64/citgm_tmp/92109405-f794-4108-8120-36349a9efdfa/gulp/node_modules/natives/index.js:110:12)
     at fs.js:57:28
     at req_ (/home/iojs/build/workspace/citgm-smoker/nodes/ubuntu1604-64/citgm_tmp/92109405-f794-4108-8120-36349a9efdfa/gulp/node_modules/natives/index.js:137:5)
     at Object.req [as require] (/home/iojs/build/workspace/citgm-smoker/nodes/ubuntu1604-64/citgm_tmp/92109405-f794-4108-8120-36349a9efdfa/gulp/node_modules/natives/index.js:54:10)
     at Object.<anonymous> (/home/iojs/build/workspace/citgm-smoker/nodes/ubuntu1604-64/citgm_tmp/92109405-f794-4108-8120-36349a9efdfa/gulp/node_modules/graceful-fs/fs.js:1:99)

edit: is also failing on Debian 8 x64, Debian 8 x86, Debian 9, and Fedora 26

@devsnek devsnek added the semver-major PRs that contain breaking changes and should be released in the next major version. label Aug 9, 2018
@jasnell
Copy link
Member

jasnell commented Aug 9, 2018

The failure in gulp makes sense given it's dependency on natives, which uses process.binding() or {} as a fallback. Generally speaking, anything that depends on natives is going to be broken by the transition away from process.binding(), which is one of the fundamental problems we're going to have moving forward with this transition.

jasnell
jasnell requested changes Aug 12, 2018
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

devsnek commented Aug 12, 2018

@jasnell this is okay with #22269 right?

@jasnell
Copy link
Member

jasnell commented Aug 12, 2018

Potentially yes. We should still be careful.

jdalton
jdalton suggested changes Aug 13, 2018
View reviewed changes
Copy link
Member

@jdalton jdalton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Holding removals until migration is ironed out

@jasnell
Copy link
Member

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 <jasnell@gmail.com>
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'));

@starkwang starkwang force-pushed the url-internal-binding branch from a820ce5 to a27fc75 Compare August 15, 2018 15:30
@starkwang
Copy link
Contributor Author

starkwang commented Aug 15, 2018

The branch has been rebased on master and applied the patch.

@starkwang
Copy link
Contributor Author

starkwang commented Aug 15, 2018

Umm... I can't open a new CI for this because it is blocked by nodejs/build#1442.

@jasnell
Copy link
Member

jasnell commented Aug 15, 2018

Yep, no worries... once the release happens the CI will be unrestricted again.

@starkwang
Copy link
Contributor Author

starkwang commented Aug 16, 2018

New CI: https://ci.nodejs.org/job/node-test-pull-request/16488/

@joyeecheung joyeecheung mentioned this pull request Aug 17, 2018
Closed
@starkwang starkwang force-pushed the url-internal-binding branch from a27fc75 to 6b79af7 Compare August 20, 2018 03:05
@starkwang
Copy link
Contributor Author

starkwang commented Aug 20, 2018

Rebased branch to resolve conflicts.

@jasnell
Copy link
Member

jasnell commented Aug 20, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/16618/

@jasnell
Copy link
Member

jasnell commented Aug 20, 2018

Ugh, another unrelated CI failure... resuming: https://ci.nodejs.org/job/node-test-pull-request/16621/

@BridgeAR
Copy link
Member

BridgeAR commented Sep 5, 2018

This needs a rebase.

@starkwang starkwang force-pushed the url-internal-binding branch from 6b79af7 to 1f62167 Compare September 6, 2018 14:48
@starkwang
Copy link
Contributor Author

starkwang commented Sep 6, 2018

Rebased and opened a new CI: https://ci.nodejs.org/job/node-test-pull-request/17058/

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 6, 2018
@starkwang
Copy link
Contributor Author

starkwang commented Sep 7, 2018

Landed in e917a23.

@starkwang starkwang closed this Sep 7, 2018
starkwang added a commit that referenced this pull request Sep 7, 2018
@starkwang
url: move process.binding('url') to internalBinding
e917a23 
PR-URL: #22204
Refs: #22160
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@trivikr trivikr trivikr approved these changes

+4 more reviewers

@jdalton jdalton jdalton approved these changes

@apapirovski apapirovski apapirovski approved these changes

@TimothyGu TimothyGu TimothyGu approved these changes

@devsnek devsnek devsnek approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. semver-major PRs that contain breaking changes and should be released in the next major version. whatwg-url Issues and PRs related to the WHATWG URL implementation.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@starkwang @nodejs-github-bot @maclover7 @jasnell @devsnek @BridgeAR @jdalton @apapirovski @TimothyGu @trivikr