-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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
js_stream: move process.binding('js_stream') to internalBinding #22239
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with good CITGM run
CI: https://ci.nodejs.org/job/node-test-pull-request/16344/ |
There was a problem hiding this 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
@antsmartian ... when you get a moment, can you rebase this from 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('js_stream')` 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', 'js_stream']);
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('js_stream')); |
@jasnell Now it should be good to go.. Thanks.. |
@antsmartian ... this will need another rebase now that some of the other conversions to internalBinding have landed. |
@jasnell Taken care, guess it should be all good now. |
Just noticed that this used a merge commit and it's failing CI because of it. Can you squash this down to a single commit and use rebase to catch it up to master. We don't use merge commits at all. |
79a2ed6
to
7b888fa
Compare
@jasnell Sorry, didn't notice that. Now its all green.. thanks for your support. |
@antsmartian ... unfortunately this will need another rebase |
@jasnell Done. |
And, yet another rebase is necessary … sorry :( |
@addaleax No worries, I have an another PR which also needs rebase, will do it tonight and ping you. |
97b7369
to
77a2399
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is one more usage in lib/internal/wrap_js_stream.js
, do we not want to change that as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with @lundibundi s comment addressed.
Yet again conflicts, let me rebase it tonight.. sorry folks.. |
77a2399
to
732e502
Compare
Landed in dcc0c2c |
PR-URL: #22239 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: John-David Dalton <john.david.dalton@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@addaleax Thanks ! |
Migration from
process.binding
tointernalBinding
(see : #22160)Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes