Skip to content

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

Merged
refack merged 1 commit into
nodejs:masterfrom
codebytere:getcurrent-wrapper
Oct 20, 2018
Merged

src: add public wrapper for Environment::GetCurrent#23676
refack merged 1 commit into
nodejs:masterfrom
codebytere:getcurrent-wrapper

Conversation

@codebytere

@codebytere codebytere commented Oct 15, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

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

nodejs-github-bot commented Oct 15, 2018

Copy link
Copy Markdown
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
Comment thread src/node.h Outdated
@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

addaleax commented Oct 15, 2018

Copy link
Copy Markdown
Member

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

refack
refack reviewed Oct 15, 2018
View reviewed changes
Comment thread src/node.h Outdated

@refack refack Oct 15, 2018

Copy link
Copy Markdown
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

@addaleax addaleax Oct 15, 2018

Copy link
Copy Markdown
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.

@refack refack Oct 15, 2018

Copy link
Copy Markdown
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 🤷‍♂️)

@refack refack Oct 15, 2018

Copy link
Copy Markdown
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

addaleax commented Oct 15, 2018

Copy link
Copy Markdown
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

gireeshpunathil commented Oct 16, 2018

Copy link
Copy Markdown
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

@joyeecheung joyeecheung left a comment •
edited
Loading

Copy link
Copy Markdown
Member

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

joyeecheung commented Oct 16, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

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

addaleax commented Oct 16, 2018

Copy link
Copy Markdown
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

refack commented Oct 20, 2018

Copy link
Copy Markdown
Contributor

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 <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
@refack refack merged commit 0feb21f into nodejs:master Oct 20, 2018
@Trott

Trott commented Oct 20, 2018

Copy link
Copy Markdown
Member

(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 <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
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 <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Signed-off-by: Beth Griggs <[email protected]>
@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

@addaleax addaleax addaleax approved these changes

@joyeecheung joyeecheung joyeecheung left review comments

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

+1 more reviewer

@refack refack refack approved these changes

Reviewers whose approvals may not affect merge requirements

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