Skip to content

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

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

Merged
merged 1 commit into from
Oct 20, 2018
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/node.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2760,6 +2760,11 @@ void FreeEnvironment(Environment* env) {
}


Environment* GetCurrentEnvironment(Local<Context> context) {
return Environment::GetCurrent(context);
}


MultiIsolatePlatform* GetMainThreadMultiIsolatePlatform() {
return v8_platform.Platform();
}
Expand Down
3 changes: 3 additions & 0 deletions src/node.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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...


// This returns the MultiIsolatePlatform used in the main thread of Node.js.
// If NODE_USE_V8_PLATFORM haven't been defined when Node.js was built,
// it returns nullptr.
Expand Down
3 changes: 3 additions & 0 deletions test/cctest/test_environment.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,13 +80,16 @@ TEST_F(EnvironmentTest, NonNodeJSContext) {
node::Environment* env = *test_env;
EXPECT_EQ(node::Environment::GetCurrent(isolate_), env);
EXPECT_EQ(node::Environment::GetCurrent(env->context()), env);
EXPECT_EQ(node::GetCurrentEnvironment(env->context()), env);

v8::Local<v8::Context> context = v8::Context::New(isolate_);
EXPECT_EQ(node::Environment::GetCurrent(context), nullptr);
EXPECT_EQ(node::GetCurrentEnvironment(context), nullptr);
EXPECT_EQ(node::Environment::GetCurrent(isolate_), env);

v8::Context::Scope context_scope(context);
EXPECT_EQ(node::Environment::GetCurrent(context), nullptr);
EXPECT_EQ(node::GetCurrentEnvironment(context), nullptr);
EXPECT_EQ(node::Environment::GetCurrent(isolate_), nullptr);
}

Expand Down