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

bootstrapper: move internalBinding to NativeModule wrapper #23025

Closed
wants to merge 1 commit into from

Conversation

@devsnek
Copy link
Member

commented Sep 22, 2018

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

@devsnek devsnek requested review from addaleax and joyeecheung Sep 22, 2018

@addaleax
Copy link
Member

left a comment

Thanks for moving this back!

@targos
targos approved these changes Sep 22, 2018
@devsnek

This comment has been minimized.

Copy link
Member Author

commented Sep 22, 2018

@jasnell
Copy link
Member

left a comment

thank you! :-)

you just crossed item #3 off my standing todo list 👍

@lpinca
lpinca approved these changes Sep 22, 2018
@@ -294,7 +294,7 @@
const requireFn = this.id.startsWith('internal/deps/') ?
NativeModule.requireForDeps :
NativeModule.require;
fn(this.exports, requireFn, this, process);
fn(this.exports, requireFn, this, process, internalBinding);

This comment has been minimized.

Copy link
@joyeecheung

joyeecheung Sep 22, 2018

Member

IIRC, it should be safe to remove internalBinding from loaderExports by now?

This comment has been minimized.

Copy link
@devsnek

devsnek Sep 22, 2018

Author Member

bootstrap/node.js uses it, so we can't

@refack

This comment has been minimized.

Copy link
Member

commented Sep 23, 2018

I would like to ask to have some context. There is no description in the OP or the commit message.
Is there a reference to discussion? Is this reverting something?

@devsnek

This comment has been minimized.

Copy link
Member Author

commented Sep 23, 2018

@refack i suppose this sorta reverts a part of 2a9eb31.

there isn't really any larger discussion though. its just annoying to have to require internal/binding/loader to grab our internal bindings. we use them often enough to put them in the wrapper.

@refack

This comment has been minimized.

Copy link
Member

commented Sep 23, 2018

sorta reverts a part of 2a9eb31.
we use them often enough to put them in the wrapper.

@devsnek Thank you. That's sort of what I was looking for. Could you put it in the commit message for future generations.

@devsnek devsnek force-pushed the devsnek:refactor/internalbinding branch from ecb3257 to 8aec3be Sep 23, 2018

@devsnek

This comment has been minimized.

Copy link
Member Author

commented Sep 23, 2018

@refack
refack approved these changes Sep 23, 2018
Copy link
Member

left a comment

Thank you!

@BridgeAR
Copy link
Member

left a comment

❤️

@devsnek devsnek force-pushed the devsnek:refactor/internalbinding branch from 8aec3be to cbbd309 Sep 25, 2018

@devsnek

This comment has been minimized.

Copy link
Member Author

commented Sep 25, 2018

@devsnek devsnek force-pushed the devsnek:refactor/internalbinding branch from cbbd309 to 3db0de9 Sep 25, 2018

@danbev

This comment has been minimized.

Copy link
Member

commented Sep 26, 2018

@danbev

This comment has been minimized.

Copy link
Member

commented Oct 4, 2018

@devsnek Would you be able to rebase this? Sorry about the delay, CI has not been very happy the last week but I'll try to re-run it and land this as soon as possible.

bootstrapper: move internalBinding to NativeModule wrapper
internalBinding is used so often that it should just automatically be
available for usage in internals.

Refs: 2a9eb31

@devsnek devsnek force-pushed the devsnek:refactor/internalbinding branch from d9391fe to 062f1de Oct 4, 2018

@danbev

This comment has been minimized.

Copy link
Member

commented Oct 4, 2018

@danbev

This comment has been minimized.

Copy link
Member

commented Oct 4, 2018

Landed in e7f710c.

@danbev danbev closed this Oct 4, 2018

danbev added a commit that referenced this pull request Oct 4, 2018
bootstrapper: move internalBinding to NativeModule
internalBinding is used so often that it should just automatically be
available for usage in internals.

PR-URL: #23025
Refs: 2a9eb31
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

@targos targos added this to Don't land (for now) in v10.x Oct 6, 2018

@addaleax

This comment has been minimized.

Copy link
Member

commented Oct 14, 2018

@targos @devsnek dont-land-on-v10.x? This should be backported, imo, as anything else is going to be a major backporting pain for LTS… especially since it essentially brings our source code back to a state in which it has been before

@devsnek

This comment has been minimized.

Copy link
Member Author

commented Oct 14, 2018

This should be fine to include in 10.x 🤷

@targos

This comment has been minimized.

Copy link
Member

commented Oct 14, 2018

Feel free to backport. I felt like it would be less work to fix a few trivial conflicts from time to time than backporting this big change.

@addaleax

This comment has been minimized.

Copy link
Member

commented Oct 14, 2018

It doesn’t cherry-pick cleanly (suprise!). @devsnek Do you want to backport or should I?

@devsnek

This comment has been minimized.

Copy link
Member Author

commented Oct 14, 2018

If you're willing that would be great, I don't have access to a checkout of node until tomorrow anyway.

addaleax added a commit to addaleax/node that referenced this pull request Oct 14, 2018
bootstrapper: move internalBinding to NativeModule
internalBinding is used so often that it should just automatically be
available for usage in internals.

PR-URL: nodejs#23025
Refs: nodejs@2a9eb31
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
addaleax added a commit to addaleax/node that referenced this pull request Oct 14, 2018
bootstrapper: move internalBinding to NativeModule
internalBinding is used so often that it should just automatically be
available for usage in internals.

PR-URL: nodejs#23025
Refs: nodejs@2a9eb31
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

@targos targos moved this from Don't land (for now) to Backported in v10.x Oct 16, 2018

jasnell added a commit that referenced this pull request Oct 17, 2018
bootstrapper: move internalBinding to NativeModule
internalBinding is used so often that it should just automatically be
available for usage in internals.

PR-URL: #23025
Refs: 2a9eb31
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
addaleax added a commit that referenced this pull request Oct 20, 2018
bootstrapper: move internalBinding to NativeModule
internalBinding is used so often that it should just automatically be
available for usage in internals.

PR-URL: #23025
Refs: 2a9eb31
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

Backport-PR-URL: #23661
Backport-Reviewed-By: Gus Caplan <me@gus.host>
Backport-Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Backport-Reviewed-By: Michaël Zasso <targos@protonmail.com>
Backport-Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Backport-Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@joyeecheung joyeecheung referenced this pull request Oct 24, 2018
4 of 4 tasks complete
MylesBorins added a commit that referenced this pull request Oct 30, 2018
bootstrapper: move internalBinding to NativeModule
internalBinding is used so often that it should just automatically be
available for usage in internals.

PR-URL: #23025
Refs: 2a9eb31
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

Backport-PR-URL: #23661
Backport-Reviewed-By: Gus Caplan <me@gus.host>
Backport-Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Backport-Reviewed-By: Michaël Zasso <targos@protonmail.com>
Backport-Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Backport-Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@codebytere codebytere referenced this pull request Nov 27, 2018
rvagg added a commit that referenced this pull request Nov 28, 2018
bootstrapper: move internalBinding to NativeModule
internalBinding is used so often that it should just automatically be
available for usage in internals.

PR-URL: #23025
Refs: 2a9eb31
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

Backport-PR-URL: #23661
Backport-Reviewed-By: Gus Caplan <me@gus.host>
Backport-Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Backport-Reviewed-By: Michaël Zasso <targos@protonmail.com>
Backport-Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Backport-Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins added a commit that referenced this pull request Nov 29, 2018
bootstrapper: move internalBinding to NativeModule
internalBinding is used so often that it should just automatically be
available for usage in internals.

PR-URL: #23025
Refs: 2a9eb31
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

Backport-PR-URL: #23661
Backport-Reviewed-By: Gus Caplan <me@gus.host>
Backport-Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Backport-Reviewed-By: Michaël Zasso <targos@protonmail.com>
Backport-Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Backport-Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@codebytere codebytere referenced this pull request Nov 29, 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.