src: add public wrapper for Environment::GetCurrent #23676
Conversation
nodejs-github-bot
added
the
c++
codebytere
added
the
embedding
addaleax
added
the
semver-minor
| @@ -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); | |||
There was a problem hiding this comment.
A question: What is the benefit of this without the Environment class declaration, which is internal?
Line 25 in 1a2cf66
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 🤷♂️)
There was a problem hiding this comment.
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
added
the
author ready
|
I agree that What it will take to make Environment class directly consumable by embedders? |
There was a problem hiding this comment.
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?
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? |
A 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
added
author ready
PR-URL: #23676 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>
PR-URL: #23676 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>
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]>
Follow-up for #23672; this PR adds a public wrapper for
Environment::GetCurrentin order to allow embedders like Electron access to the current environment without having to use private APIs and experience issues around private members likeEnvironment::kNodeContextTagPtr.Also adds a test to ensure that this new wrapper does not break on future changes.
/cc @addaleax
Checklist
make -j4 testpasses