Skip to content

src: expose Environment::kNodeContextTagPtr#23672

Closed
codebytere wants to merge 1 commit intonodejs:masterfrom
codebytere:expose-kNodeContextTagPtr
Closed

src: expose Environment::kNodeContextTagPtr#23672
codebytere wants to merge 1 commit intonodejs:masterfrom
codebytere:expose-kNodeContextTagPtr

Conversation

@codebytere
Copy link
Member

@codebytere codebytere commented Oct 15, 2018

This PR exposes Environment::kNodeContextTagPtr. This was introduced a while back in #19134, and then recently introduced into Environment::GetCurrent.

Electron uses Environment::GetCurrent in several places (such as here), and since kNodeContextTagPtr wasn't exposed, we began to run into undefined symbol errors.

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Oct 15, 2018

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

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Oct 15, 2018
@addaleax addaleax added the embedding Issues and PRs related to embedding Node.js in another project. label Oct 15, 2018
@addaleax
Copy link
Member

addaleax commented Oct 15, 2018

Uh … this is implementation details and I’d really like to avoid exposing it.

Environment::GetCurrent() isn’t public API either, fwiw.

How about exposing a public wrapper for Environment::GetCurrent() instead?

@addaleax
Copy link
Member

addaleax commented Oct 15, 2018

@codebytere Would you like to implement the suggestion above? I think it could be something like a Environment* GetNodeEnvironment(v8::Local<v8::Context>) function, maybe near GetCurrentEventLoop() in the hearders/source code?

@codebytere codebytere closed this Oct 15, 2018
@codebytere codebytere deleted the expose-kNodeContextTagPtr branch October 15, 2018 18:27
@codebytere codebytere mentioned this pull request Oct 15, 2018
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. embedding Issues and PRs related to embedding Node.js in another project.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@codebytere @nodejs-github-bot @addaleax