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

js_stream: move process.binding('js_stream') to internalBinding #22239

Closed
wants to merge 1 commit into from

Conversation

@antsmartian
Copy link
Contributor

antsmartian commented Aug 10, 2018

Migration from process.binding to internalBinding (see : #22160)

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Copy link
Member

jasnell left a comment

LGTM with good CITGM run

@maclover7

This comment has been minimized.

Copy link
Member

maclover7 commented Aug 10, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/16344/
(will start a CITGM run after the current job is finished)

@jasnell jasnell mentioned this pull request Aug 10, 2018
32 of 33 tasks complete
@maclover7

This comment has been minimized.

Copy link
Member

maclover7 commented Aug 10, 2018

Copy link
Member

jdalton left a comment

Holding removals until migration is ironed out

@jasnell

This comment has been minimized.

Copy link
Member

jasnell commented Aug 15, 2018

@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'));
@antsmartian

This comment has been minimized.

Copy link
Contributor Author

antsmartian commented Aug 15, 2018

@jasnell Now it should be good to go.. Thanks..

@jasnell

This comment has been minimized.

Copy link
Member

jasnell commented Aug 19, 2018

@antsmartian ... this will need another rebase now that some of the other conversions to internalBinding have landed.

@antsmartian

This comment has been minimized.

Copy link
Contributor Author

antsmartian commented Aug 20, 2018

@jasnell Taken care, guess it should be all good now.

@jasnell

This comment has been minimized.

Copy link
Member

jasnell commented Aug 20, 2018

@jasnell

This comment has been minimized.

Copy link
Member

jasnell commented Aug 20, 2018

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.

@antsmartian antsmartian force-pushed the antsmartian:js_stream branch 2 times, most recently from 79a2ed6 to 7b888fa Aug 21, 2018
@antsmartian

This comment has been minimized.

Copy link
Contributor Author

antsmartian commented Aug 21, 2018

@jasnell Sorry, didn't notice that. Now its all green.. thanks for your support.

@jasnell

This comment has been minimized.

@jasnell

This comment has been minimized.

Copy link
Member

jasnell commented Aug 24, 2018

@antsmartian ... unfortunately this will need another rebase

@antsmartian

This comment has been minimized.

Copy link
Contributor Author

antsmartian commented Aug 26, 2018

@jasnell Done.

@addaleax addaleax removed the author ready label Sep 2, 2018
@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Sep 2, 2018

And, yet another rebase is necessary … sorry :(

@antsmartian

This comment has been minimized.

Copy link
Contributor Author

antsmartian commented Sep 3, 2018

@addaleax No worries, I have an another PR which also needs rebase, will do it tonight and ping you.

@antsmartian antsmartian force-pushed the antsmartian:js_stream branch from 97b7369 to 77a2399 Sep 3, 2018
@antsmartian

This comment has been minimized.

Copy link
Contributor Author

antsmartian commented Sep 3, 2018

@addaleax Rebased it, now it should be good I guess.. cc @jasnell

Copy link
Member

lundibundi left a comment

There is one more usage in lib/internal/wrap_js_stream.js, do we not want to change that as well?

Copy link
Member

BridgeAR left a comment

LGTM with @lundibundi s comment addressed.

@antsmartian

This comment has been minimized.

Copy link
Contributor Author

antsmartian commented Sep 11, 2018

Yet again conflicts, let me rebase it tonight.. sorry folks..

@antsmartian antsmartian force-pushed the antsmartian:js_stream branch from 77a2399 to 732e502 Sep 15, 2018
@antsmartian

This comment has been minimized.

Copy link
Contributor Author

antsmartian commented Sep 15, 2018

Ping @nodejs/collaborators . Can we merge this please, before I get another conflict :) Thanks!

Edit: Looks like I can't ping teams. So pinging few people : @BridgeAR @addaleax @jasnell @trivikr

@trivikr

This comment has been minimized.

Copy link
Member

trivikr commented Sep 16, 2018

@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Sep 17, 2018

@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Sep 18, 2018

Landed in dcc0c2c

@addaleax addaleax closed this Sep 18, 2018
addaleax added a commit that referenced this pull request Sep 18, 2018
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>
@antsmartian

This comment has been minimized.

Copy link
Contributor Author

antsmartian commented Sep 18, 2018

@addaleax Thanks !

@antsmartian antsmartian deleted the antsmartian:js_stream branch Sep 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.