I need a bit more detail than this. A Node.js environment . . . with its own global scope? That can have separate NODE_OPTIONS? Is it CommonJS or ESM, or either?

To others’ points, how does this differ from Realm?

A Realm (in specs terms) does not support ESM, require and all Node.js core modules. It's like a Realm, but with all the Node.js stuff.

This is really any module specifier that can be passed to require, right? So not just filenames but also bare specifiers like lodash? Or node:fs?

this is just require, yes.

Since this is the same as module.createRequire(), we should copy some of the docs from that API. For example, filename can also be a URL.

What is the signature of the returned function? Does it return a promise? Does it match the signature of import(), where the second argument is an options bag (like { assert: { type: 'json' } })?

nitpick. makes more sense IMO

This doesn't seem to be a good idea - I think so far we've been assuming in the code base that there is a 1:1 relationship between the environment and the isolate, things can be subtly broken once we have n:1. It would be cleaner if we follow the Realm approach and just split out states in the Environment that are unique to each context/realm to a subclass of node::Realm (or maybe it is already just node::PrincipalRealm).

I think so far we've been assuming in the code base that there is a 1:1 relationship between the environment and the isolate, things can be subtly broken once we have n:1.

Quite a few of the crashes I've been fighting with this approach are due to that. I'm using the reference to env to clean up all the handles. Otherwise, we will have bad crashes after stop() is called (which is why I'm doing this in core). Using a node::PrincipalReam would require removing the deliberate stop() method: how would we shut it down?

The other question I have if we dith the CreateEnvironment call, how do we guarantee some level of isolation?

You can just move the handles into the Realm instead. Basically, just move things that should belong to individual realms instead of being shared across them to the realm, and do the setup/cleanup on a per-realm basis, which is what we've been trying to do with the ShadowRealm integration.

You can just move the handles into the Realm instead.

How would you do that?

As an example HandleWrap add things to the Environment here:

Should I try to move that list from the Env to the Realm?

Yes, just make that realm()->handle_wrap_queue()->PushBack(this). That's what we've been doing to support other BaseObjects in ShadowRealms, we just haven't got to HandleWrap yet. If our intent is to make individual realms have their own sets of handles etc., we'd have to move them into realms and stop mixing them in one giant Environment that contains per-thread information anyway.

Yeah, tracking handle wraps and request wraps by realms is the plan for shadow realm integration. It's not the current focus yet.

However, moving the list from the Environment to the Realm breaks the postmortem diagnostics since tools like llnode are built on top of the Environment::handle_wrap_queue structure: https://github.com/nodejs/node/blob/main/src/node_postmortem_metadata.cc#L23

An option can be tracking the handle wraps and request wraps by both the Env and its creation Realm.

I think we don't need to worry much about the postmortem diagnostics data, we only need to leave the offset of the queues within Realms and the tools would then figure out how to adapt to newer versions of Node.js. It's never guaranteed that we'd never change the layout of our internals, only that when we do, we still leave some information in the binary (the offsets) for these tools to figure out how to extract information from a core dump / process memory.

My understanding is that we won't be able to have a process object without a new Node.js Environment, right?

May we land this PR and work to remove CreateEnvironment in follow-on work?

I think so far we've been assuming in the code base that there is a 1:1 relationship between the environment and the isolate

That that's not the case is precisely the difference between Environment and IsolateData, though. Just because the Node.js CLI doesn't create multiple Environment instances per Isolate doesn't mean that it's not semantically correct to do so.

That that's not the case is precisely the difference between Environment and IsolateData, though. Just because the Node.js CLI doesn't create multiple Environment instances per Isolate doesn't mean that it's not semantically correct to do so.

I don't think in practice we have really been writing the code that way. There are some places where we configure V8 isolate hooks (e.g. the ones in Environment::InitializeDiagnostics) with the current Environment as data to be used in the callback. The async hooks where things like Environment::GetCurrent(isolate)->trigger_async_id() is used a lot don't look particularly robust against a multi-Environment architecture either, and there are probably more places than I can think of off the top of my head..

I do think the docs should clarify the difference between this and a ShadowRealm.

I would also like to understand the differences (and similarities) between this and a worker. Because they look very similar. For example, does a realm have an event loop? Does it share globals? (I'm assuming yes and no?)

Could this be simplified to noderealm.import('my-module')? As in, are we able to infer the module parent (import.meta.url) from within createImport, and if so, is there ever a reason we wouldn’t want to import relative to that?

I don't think retrieving a function caller's location reliably is possible.

And for all following.

Why 100 and not, say, 10?

See the comment above. Completely arbitrary, it could have been 42. I feel retrying 10 times per second is a good compromise, retrying 100 times per second might be too much.

I will refactor this to have a better solution for this active wait.

(After this lands)

Should NodeRealm inherit from BaseObject? This seems pretty similar to BaseObject's work.

Yes. Only reason I didn't do this in synchronous-worker is because it was built outside of Node.js core.

Can we create the NodeRealm with a specifier?

interface NodeRealm {
  constructor(specifier: string);
  import(specifier, importAssertions): ModuleNamespace;
}

In this way, we can get rid of the higher order function createImport and makes the class method more aligned with ShadowRealm.prototype.importValue:

import { NodeRealm } from 'node:vm';
const nodeRealm = new NodeRealm(import.meta.url);
const { myAsyncFunction } = await nodeRealm.import('my-module');
console.log(await myAsyncFunction());

Non-blocking nit: not really a fan of w_ as a member variable name. Something more descriptive would be ideal. realm_ perhaps?

assert()s should be turned into CHECKs

This should be using onexit_string()