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 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

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Aug 10, 2018
@addaleax addaleax added the semver-major PRs that contain breaking changes and should be released in the next major version. label Aug 10, 2018
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.

LGTM with good CITGM run

@maclover7
Copy link
Contributor

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

@maclover7
Copy link
Contributor

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

@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
Copy link
Contributor Author

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

@jasnell
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
Copy link
Contributor Author

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

@jasnell
Copy link
Member

jasnell commented Aug 20, 2018

@jasnell
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 js_stream branch 2 times, most recently from 79a2ed6 to 7b888fa Compare August 21, 2018 01:39
@antsmartian
Copy link
Contributor Author

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

@jasnell
Copy link
Member

jasnell commented Aug 21, 2018

@jasnell jasnell added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 21, 2018
@jasnell
Copy link
Member

jasnell commented Aug 24, 2018

@antsmartian ... unfortunately this will need another rebase

@antsmartian
Copy link
Contributor Author

@jasnell Done.

@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 2, 2018
@addaleax
Copy link
Member

addaleax commented Sep 2, 2018

And, yet another rebase is necessary … sorry :(

@antsmartian
Copy link
Contributor Author

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

@antsmartian
Copy link
Contributor Author

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

Copy link
Member

@lundibundi lundibundi left a 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?

Copy link
Member

@BridgeAR BridgeAR left a 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.

@antsmartian
Copy link
Contributor Author

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

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

trivikr commented Sep 16, 2018

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

@trivikr trivikr added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 16, 2018
@addaleax
Copy link
Member

@addaleax
Copy link
Member

Landed in dcc0c2c

@addaleax addaleax closed this Sep 18, 2018
addaleax pushed 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
Copy link
Contributor Author

@addaleax Thanks !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.