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

Tracking Issue: process.binding to internalBinding #22160

Closed
32 of 33 tasks
jasnell opened this issue Aug 6, 2018 · 17 comments
Closed
32 of 33 tasks

Tracking Issue: process.binding to internalBinding #22160

jasnell opened this issue Aug 6, 2018 · 17 comments
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. help wanted Issues that need assistance from volunteers or PRs that need help to proceed.

Comments

@jasnell
Copy link
Member

jasnell commented Aug 6, 2018

Moving from process.binding() to internalBinding() will be a significant effort. There are hundreds of process.binding() uses inside core and many many userland references. It's going to take a while to convert everything.

TODO

  • inspector (this one is quite non-trivial... will require additional thought)

Done

@devsnek
Copy link
Member

devsnek commented Aug 6, 2018

updated to include all usage of NODE_BUILTIN_MODULE_CONTEXT_AWARE

@devsnek devsnek added help wanted Issues that need assistance from volunteers or PRs that need help to proceed. c++ Issues and PRs that require attention from people who are familiar with C++. labels Aug 6, 2018
@jasnell
Copy link
Member Author

jasnell commented Aug 6, 2018

I've got uv in progress now

@devsnek
Copy link
Member

devsnek commented Aug 6, 2018

@ChALkeR would it be possible to get info about the relative usage of all of these? i'm assuming there are some like serdes we can kill right away and others like util that will take longer.

@jasnell
Copy link
Member Author

jasnell commented Aug 6, 2018

I tried doing zlib but it broke npm.
I tried doing config but there's a buffer test that isn't feasible if config is moved. Planning to refactor

@maclover7
Copy link
Contributor

Just double checking, should all migration PRs should be marked as semver-major?

@jasnell
Copy link
Member Author

jasnell commented Aug 6, 2018

Unfortunately that's likely the best thing.

@jasnell
Copy link
Member Author

jasnell commented Aug 6, 2018

For some of this it's likely going to be quite tricky to have a proper migration strategy in place given that moving something over to internalBinding() means that the process.binding() version just stops working. We're going to have to take it very slow and work on migrating the bits with the smallest immediate impact and working up from there.

devsnek added a commit to devsnek/node that referenced this issue Aug 8, 2018
PR-URL: nodejs#22161
Refs: nodejs#22160
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@pratik0509
Copy link
Contributor

I would also like to contribute here. Can you please give me some heads up?

@jmannanc
Copy link

Hey,

I've been trying to move buffer to internalBinding to try and help out and I think I've gotten close but make -j4 test keeps giving these errors: https://hastebin.com/ecucikeraj.cs

You can see the branch that I'm working on here. Any help on maybe how to fix these errors/why they are happening would be very much appreciated 😄. Sorry for the bother if my changes are incorrect or this is not allowed.

danbev pushed a commit that referenced this issue Dec 13, 2018
In places of process.binding('inspector'), migrate code to adapt
internalBinding.

PR-URL: #24931
Refs: #22160
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@starkwang
Copy link
Contributor

Now the inspector module is the only one left to be converted. Should we open a new issue to track it ? @jasnell

@BeniCheni
Copy link
Contributor

Hi, @starkwang. it appears that #24931 took care of inspector.

@jasnell
Copy link
Member Author

jasnell commented Jan 6, 2019

I think we can close this now.

@jasnell jasnell closed this as completed Jan 6, 2019
BridgeAR pushed a commit to BridgeAR/node that referenced this issue Jan 9, 2019
In places of process.binding('inspector'), migrate code to adapt
internalBinding.

PR-URL: nodejs#24931
Refs: nodejs#22160
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
BridgeAR pushed a commit that referenced this issue Jan 10, 2019
In places of process.binding('inspector'), migrate code to adapt
internalBinding.

PR-URL: #24931
Refs: #22160
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
joyeecheung pushed a commit to joyeecheung/node that referenced this issue Jan 11, 2019
Refs: nodejs#22160

PR-URL: nodejs#22478
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
joyeecheung pushed a commit to joyeecheung/node that referenced this issue Jan 11, 2019
PR-URL: nodejs#23400
Refs: nodejs#22160
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
joyeecheung pushed a commit to joyeecheung/node that referenced this issue Jan 11, 2019
In places of process.binding('inspector'), migrate code to adapt
internalBinding.

PR-URL: nodejs#24931
Refs: nodejs#22160
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
addaleax pushed a commit that referenced this issue Jan 14, 2019
In places of process.binding('inspector'), migrate code to adapt
internalBinding.

PR-URL: #24931
Refs: #22160
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
addaleax pushed a commit that referenced this issue Jan 14, 2019
Refs: #22160

PR-URL: #22478
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>

Backport-PR-URL: #25446
addaleax pushed a commit that referenced this issue Jan 14, 2019
PR-URL: #23400
Refs: #22160
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>

Backport-PR-URL: #25446
Reviewed-By: Anna Henningsen <[email protected]>
addaleax pushed a commit that referenced this issue Jan 14, 2019
In places of process.binding('inspector'), migrate code to adapt
internalBinding.

PR-URL: #24931
Refs: #22160
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>

Backport-PR-URL: #25446
refack pushed a commit to refack/node that referenced this issue Jan 14, 2019
PR-URL: nodejs#23400
Refs: nodejs#22160
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
refack pushed a commit to refack/node that referenced this issue Jan 14, 2019
In places of process.binding('inspector'), migrate code to adapt
internalBinding.

PR-URL: nodejs#24931
Refs: nodejs#22160
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
BridgeAR pushed a commit to BridgeAR/node that referenced this issue Jan 16, 2019
In places of process.binding('inspector'), migrate code to adapt
internalBinding.

PR-URL: nodejs#24931
Refs: nodejs#22160
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
BridgeAR pushed a commit to BridgeAR/node that referenced this issue Jan 16, 2019
Refs: nodejs#22160

PR-URL: nodejs#22478
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>

Backport-PR-URL: nodejs#25446
BridgeAR pushed a commit to BridgeAR/node that referenced this issue Jan 16, 2019
PR-URL: nodejs#23400
Refs: nodejs#22160
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>

Backport-PR-URL: nodejs#25446
Reviewed-By: Anna Henningsen <[email protected]>
BridgeAR pushed a commit to BridgeAR/node that referenced this issue Jan 16, 2019
In places of process.binding('inspector'), migrate code to adapt
internalBinding.

PR-URL: nodejs#24931
Refs: nodejs#22160
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>

Backport-PR-URL: nodejs#25446
BridgeAR pushed a commit to BridgeAR/node that referenced this issue Mar 6, 2019
Migrate various modules from using process.binding to internalBinding.

PR-URL: nodejs#24952
Refs: nodejs#22160
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
BridgeAR pushed a commit to BridgeAR/node that referenced this issue Mar 12, 2019
Migrate various modules from using process.binding to internalBinding.

PR-URL: nodejs#24952
Refs: nodejs#22160
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
BridgeAR pushed a commit that referenced this issue Mar 14, 2019
Migrate various modules from using process.binding to internalBinding.

PR-URL: #24952
Refs: #22160
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. help wanted Issues that need assistance from volunteers or PRs that need help to proceed.
Projects
None yet
Development

No branches or pull requests

9 participants