src: add public wrapper for Environment::GetCurrent

[codebytere]

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

• make -j4 test passes
• commit message follows commit guidelines

[nodejs-github-bot]

@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

[addaleax]

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

[refack]

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]

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]

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]

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]

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

[gireeshpunathil]

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]

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]

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]

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]

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

[Trott]

(Landed in 0feb21f by @refack.)