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.

Sign up for GitHub

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

Jump to bottom

src: add public wrapper for Environment::GetCurrent #23676

Merged
merged 1 commit into from
Oct 20, 2018
Merged

src: add public wrapper for Environment::GetCurrent #23676

merged 1 commit into from
Oct 20, 2018

Conversation

codebytere
Copy link
Member

@codebytere codebytere commented Oct 15, 2018 •
edited
Loading

Follow-up for #23672; this PR adds a public wrapper for Environment::GetCurrent in order to allow embedders like Electron access to the current environment without having to use private APIs and experience issues around private members like Environment::kNodeContextTagPtr.

Also adds a test to ensure that this new wrapper does not break on future changes.

/cc @addaleax

Checklist

@nodejs-github-bot
Copy link
Collaborator

@codebytere build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1253/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
@codebytere codebytere added the embedding Issues and PRs related to embedding Node.js in another project. label Oct 15, 2018
addaleax
addaleax approved these changes Oct 15, 2018
View reviewed changes
src/node.h Show resolved Hide resolved
@addaleax addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Oct 15, 2018
@addaleax
Copy link
Member

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

refack
refack reviewed Oct 15, 2018
View reviewed changes
src/node.h
@@ -265,6 +265,9 @@ NODE_EXTERN Environment* CreateEnvironment(IsolateData* isolate_data,
NODE_EXTERN void LoadEnvironment(Environment* env);
NODE_EXTERN void FreeEnvironment(Environment* env);

// This may return nullptr if context is not associated with a Node instance.
NODE_EXTERN Environment* GetCurrentEnvironment(v8::Local<v8::Context> context);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A question: What is the benefit of this without the Environment class declaration, which is internal?

node/src/env.h

Line 25 in 1a2cf66

#if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For one, you can test whether the current Context is a Node.js one or not, which is what electron does, I think.

Secondly, Environment would be the most reasonable target to improve the embedder API on, even if the class itself is not public. There are a couple of existing functions that take is as an argument, for example.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only caveat is that we need forward declarations:
https://github.com/nodejs/node/blob/0a90f943abe30a75078933f3b982de0096a4a569/src/node.h#L216-L219
(And they are exposed to internal code as well 🤷‍♂️)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that I think of it, this might be way part of the reason I'm having trouble "embedded" node into node in #23439.
I think that somehow we are violating the ODR...

@addaleax
Copy link
Member

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

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

I agree that Environment is the mot reasonable abstraction for an embedder, and in the long run, we should align all embedder's APIs around this.

What it will take to make Environment class directly consumable by embedders?

joyeecheung
joyeecheung reviewed Oct 16, 2018
View reviewed changes
Copy link
Member

@joyeecheung joyeecheung left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a test, just so that we don't accidentally break this? Maybe like EnvironmentTest in cctest/test_environment.cc (though that's testing internals so I believe we need to test the public API somewhere else)

EDIT: looks like we can just replace the Environment::GetCurrent call there?

@joyeecheung
Copy link
Member

joyeecheung commented Oct 16, 2018 •
edited
Loading

What it will take to make Environment class directly consumable by embedders?

I don't think we should just expose the internal Environment class as-is, there are too many implementation details there (at least in my understanding, it's more like a hotchpotch for hanging stuff that we can't find a better place for). But given that there is napi_env, maybe we can model a public class based on that?

@addaleax
Copy link
Member

But given that there is napi_env, maybe we can model a public class based on that?

A napi_env corresponds to v8::Context (it’s not obvious from the definition, this is has just emerged in conversation with N-API people), so it’s happening on another level.

We talked about this in the N-API meeting at the Collab Summit, and agreed that it would be better to iterate on the C++ API for now before settings something for N-API in stone.

@refack
Copy link
Contributor

refack commented Oct 20, 2018

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

@refack refack added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Oct 20, 2018
@codebytere @refack
src,test: add public wrapper for Environment::GetCurrent
0feb21f 
PR-URL: #23676
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
@refack refack merged commit 0feb21f into nodejs:master Oct 20, 2018
@Trott
Copy link
Member

Trott commented Oct 20, 2018

(Landed in 0feb21f by @refack.)

jasnell pushed a commit that referenced this pull request Oct 21, 2018
@codebytere @jasnell
src,test: add public wrapper for Environment::GetCurrent
39fcda0 
PR-URL: #23676
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
BethGriggs pushed a commit that referenced this pull request Apr 8, 2019
@codebytere @BethGriggs
src,test: add public wrapper for Environment::GetCurrent
c2dab8e 
PR-URL: #23676
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Signed-off-by: Beth Griggs <Bethany.Griggs@uk.ibm.com>
@BethGriggs BethGriggs mentioned this pull request May 1, 2019
Merged
This was referenced May 29, 2019
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@refack refack refack approved these changes

@addaleax addaleax addaleax approved these changes

@joyeecheung joyeecheung joyeecheung left review comments

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

Assignees
No one assigned
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++. embedding Issues and PRs related to embedding Node.js in another project. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@codebytere @nodejs-github-bot @addaleax @gireeshpunathil @joyeecheung @refack @Trott