src: expose node::RegisterContext to make a node managed context

[legendecas]

This allows addons to create a context, with Node.js inspector support.

The API exposed is intended to be minimum, to be used together with
node::NewContext.

[nodejs-github-bot]

Review requested:

• @nodejs/gyp

[addaleax]

The created context is identicaly to the one created with vm.createContext
in node:vm. However, node:vm does not allow an addon to unwrap the
v8::Context out of the sandbox object.

The API exposed is intended to be minimum

That kinda sounds like what we primarily need/want is an API to unwrap the v8::Context from a sandbox object – couldn't we do that instead? (It's also not super difficult to do that tbh, you could run vm.runInContext('{}', context) and do obj->GetCreationContext() on the result)

[codecov]

Codecov Report

❌ Patch coverage is 66.66667% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.06%. Comparing base (3b19867) to head (fdf9259).
⚠️ Report is 31 commits behind head on main.

Files with missing lines	Patch %	Lines
src/api/environment.cc	57.14%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #62322      +/-   ##
==========================================
- Coverage   91.68%   90.06%   -1.62%     
==========================================
  Files         361      714     +353     
  Lines      156230   225942   +69712     
  Branches    24021    42748   +18727     
==========================================
+ Hits       143232   203490   +60258     
- Misses      12729    14243    +1514     
- Partials      269     8209    +7940     

Files with missing lines	Coverage Δ
src/env.h	98.21% <100.00%> (ø)
src/node.h	92.30% <ø> (ø)
src/api/environment.cc	78.86% <57.14%> (ø)

... and 475 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[legendecas]

@addaleax: That kinda sounds like what we primarily need/want is an API to unwrap the v8::Context from a sandbox object – couldn't we do that instead?

Yeah, that's the main purpose. We could expose an API to unwrap the v8::Context out from sandbox object directly. However, I feel like it would be more straitforward to allow addons to create and manage a Node.js managed v8::Context directly, similar to node::NewContext (maybe we should encourage addons to use this new API instead of node::NewContext because it is not configured to support inspector).

[addaleax]

We could expose an API to unwrap the v8::Context out from sandbox object directly.

I still feel like that's both easier and a bit more powerful, but I'll leave it up to you 👍

[legendecas]

I still feel like that's both easier and a bit more powerful

I can add both. A node::MakeContextify could avoid jumping between JS and C++ unnecessarily for creating a context, and as a replacement of node::NewContext. And an unwrap method like you suggested could help dealing with existing vm contexts.

[joyeecheung]

Is this mostly for that NodeInspectorClient::contextCreated call?

I think alternatively we can consider just exposing an API that returns an opaque pointer to the NodeInspectorClient, and selectively expose some APIs that are stable. That might allow better inspector integration for embedders (e.g. dispatching inspector messages to Node.js more directly so that they surface nicely in the devtools through the protocol). I don't think exposing vm context to the API would hinder other vm efforts since they are a bit orthogonal, but vm context's interceptor behaviors are quite quirky, so by using that the embedders are also inheriting the quirks that are mostly only there due to a specific JS API compat. If the overall desirable thing lies in inspector instead it seems like an unnecessary baggage, a normal v8::Context + embedder defined global behavior is much less quirky than the interceptors (this is why we have vm.constants.DONT_CONTEXTIFY, because even in JS land it's less weird to just not contextify it and define what you need directly on the global proxy instead of going through the legacy interceptors).

[legendecas]

Updated the PR to expose node::RegisterContext/node::UnregisterContext (equivalent to node::Environment::AssignToContext/node::Environment::UnassignFromContext) as the minimum APIs instead.

Embedders could compose node::NewContext together with node::RegisterContext. This does not change existing API behavior (in case embedders are already using node::NewContext with conflict context slots and promise hooks).

@addaleax @joyeecheung would you mind taking a look again? Thank you!

[legendecas]

Updated the public API to use std::string_view instead.

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[legendecas]

@joyeecheung @addaleax CI passed. Would you mind taking a look again? Thanks!

[nodejs-github-bot]

Landed in d92ee51