bootstrapper: move internalBinding to NativeModule wrapper#23025

devsnek · 2018-09-22T14:52:58Z

Checklist

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

nodejs-github-bot · 2018-09-22T14:52:59Z

@devsnek build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/978/pipeline

addaleax

Thanks for moving this back!

devsnek · 2018-09-22T15:45:29Z

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

jasnell

thank you! :-)

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

joyeecheung · 2018-09-22T22:59:08Z

lib/internal/bootstrap/loaders.js

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

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

refack · 2018-09-23T00:39:39Z

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 · 2018-09-23T01:15:26Z

@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 · 2018-09-23T01:35:33Z

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 · 2018-09-23T16:24:31Z

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

refack

Thank you!

BridgeAR

❤️

devsnek · 2018-09-25T03:17:39Z

https://ci.nodejs.org/job/node-test-pull-request/17415/

danbev · 2018-09-26T03:19:43Z

Re-run of failing node-test-commit-arm-fanned and node-test-commit-linux.

danbev · 2018-10-04T03:24:04Z

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

internalBinding is used so often that it should just automatically be available for usage in internals. Refs: nodejs@2a9eb31

danbev · 2018-10-04T05:33:23Z

Rebased CI: https://ci.nodejs.org/job/node-test-pull-request/17618/

danbev · 2018-10-04T10:25:35Z

Landed in e7f710c.

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 <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>

addaleax · 2018-10-14T22:38:54Z

@targos @devsnekdont-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 · 2018-10-14T22:41:51Z

This should be fine to include in 10.x 🤷

targos · 2018-10-14T22:41:54Z

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 · 2018-10-14T22:45:31Z

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

devsnek · 2018-10-14T22:52:58Z

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

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 <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>

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 <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>

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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Backport-PR-URL: #23661 Backport-Reviewed-By: Gus Caplan <[email protected]> Backport-Reviewed-By: Richard Lau <[email protected]> Backport-Reviewed-By: Michaël Zasso <[email protected]> Backport-Reviewed-By: Joyee Cheung <[email protected]> Backport-Reviewed-By: Ruben Bridgewater <[email protected]>

devsnek requested review from addaleax and joyeecheung September 22, 2018 14:52

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Sep 22, 2018

addaleax approved these changes Sep 22, 2018
View reviewed changes

targos approved these changes Sep 22, 2018
View reviewed changes

jasnell approved these changes Sep 22, 2018
View reviewed changes

cjihrig approved these changes Sep 22, 2018
View reviewed changes

lpinca approved these changes Sep 22, 2018
View reviewed changes

joyeecheung approved these changes Sep 22, 2018
View reviewed changes

devsnek force-pushed the refactor/internalbinding branch from ecb3257 to 8aec3beCompare September 23, 2018 01:56

refack approved these changes Sep 23, 2018
View reviewed changes

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 23, 2018

BridgeAR approved these changes Sep 23, 2018
View reviewed changes

devsnek force-pushed the refactor/internalbinding branch from 8aec3be to cbbd309Compare September 25, 2018 03:17

devsnek force-pushed the refactor/internalbinding branch from cbbd309 to 3db0de9Compare September 25, 2018 03:20

thefourtheye approved these changes Sep 25, 2018
View reviewed changes

trivikr approved these changes Sep 25, 2018
View reviewed changes

bootstrapper: move internalBinding to NativeModule wrapper
062f1de
internalBinding is used so often that it should just automatically be available for usage in internals. Refs: nodejs@2a9eb31

devsnek force-pushed the refactor/internalbinding branch from d9391fe to 062f1deCompare October 4, 2018 03:46

danbev closed this Oct 4, 2018

targos added dont-land-on-v6.x labels Oct 6, 2018

addaleax added backport-requested-v10.x and removed dont-land-on-v10.x labels Oct 14, 2018

addaleax added backported-to-v10.x and removed backport-requested-v10.x labels Oct 14, 2018

addaleax mentioned this pull request Oct 14, 2018
[v10.x] bootstrapper: move internalBinding to NativeModule #23661
Closed

joyeecheung mentioned this pull request Oct 24, 2018
lib: modify NativeModule to use compileFunction #23837
Open
4 tasks

joyeecheung mentioned this pull request Nov 24, 2018
src: compile native modules and their code cache in C++ #24221
Closed
3 tasks

codebytere mentioned this pull request Nov 27, 2018
v10.13.1 proposal #24675
Closed

codebytere mentioned this pull request Nov 29, 2018
v10.14.2 proposal #24727
Merged

Uh oh!

bootstrapper: move internalBinding to NativeModule wrapper#23025

bootstrapper: move internalBinding to NativeModule wrapper #23025

Uh oh!

Conversation

devsnek commented Sep 22, 2018

Checklist

Uh oh!

nodejs-github-bot commented Sep 22, 2018

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

devsnek commented Sep 22, 2018

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

joyeecheungSep 22, 2018

Choose a reason for hiding this comment

Uh oh!

devsnekSep 22, 2018

Choose a reason for hiding this comment

Uh oh!

refack commented Sep 23, 2018

Uh oh!

devsnek commented Sep 23, 2018

Uh oh!

refack commented Sep 23, 2018

Uh oh!

devsnek commented Sep 23, 2018

Uh oh!

refack left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

devsnek commented Sep 25, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danbev commented Sep 26, 2018

Uh oh!

danbev commented Oct 4, 2018

Uh oh!

danbev commented Oct 4, 2018

Uh oh!

danbev commented Oct 4, 2018

Uh oh!

addaleax commented Oct 14, 2018

Uh oh!

devsnek commented Oct 14, 2018

Uh oh!

targos commented Oct 14, 2018

Uh oh!

addaleax commented Oct 14, 2018

Uh oh!

devsnek commented Oct 14, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

devsnek commented Sep 25, 2018•
edited
Loading