Node: `vm.Script`'s `importModuleDynamically` should get a reference to the `context` used to run it

[SimenB]

Is your feature request related to a problem? Please describe.

When implementing importModuleDynamically you don't have access to what context the script is executed with, meaning you can't pass the correct context when constructing a Module. We're also missing the filename of the Script, so resolving to the specifier passed is also not straightforward since it can be changed by .runIn*Context() calls from what I passed in to the constructor.

Describe the solution you'd like

I think adding context and filename as accessible properties on the Script instance passed to the function should work fine - it would mirror what you get when using SourceTextModule where I have access to context and identifier. It could also be passed as a third argument to the function passed in importModuleDynamically if you don't wanna change the Script instance itself.

Describe alternatives you've considered

The implementation I've gone with in the absence of such an API is to get the context again and re-use the filename passed in the constructor. This works since I also have control over how the script is executed, but that might not always be the case. Mirroring the capability of SourceTextModule would be nice, though - where in the context of the callback in importModuleDynamically there's enough information to know how to resolve the specifier being requested and in what context it should run.

[SimenB]

(vm label probably makes just as much sense as module 🙂)

[devsnek]

We should add the needed properties to scripts. Extending the importModuleDynamically callback would be unfortunate.

[SimenB]

That would mean the passed in Script must be different from the outer one, right? E.g. this works fine today

const assert = require('assert');
const vm = require('vm');

const outerScript = new vm.Script('import("woo").then(console.log)', {
  async importModuleDynamically(_, innerScript) {
    assert.ok(outerScript === innerScript);

    // the below is just to please the API in returning a module, not relevant to the point I'm making
    const module = new vm.SyntheticModule(['default'], function () {
      this.setExport('default', 'hello!');
    });

    await module.link(() => {
      throw new Error('Linker should not be called');
    });
    await module.evaluate();

    return module;
  },
});

outerScript.runInThisContext();

If innerScript were to have e.g. filename on it, that would necessarily have to be different from outerScript, I believe. Since I can run the script many times in different contexts before it completes its execution. Dunno if there's a use case for them being the same instance, tho.

These APIs are experimental, so I guess it doesn't really matter if that contract (if it even _is_ a contract and not a "coincidence") is broken. I would personally prefer an approach where it's added to Script since it more closely mirrors SourceTextModule, so I won't be arguing in favor of a third parameter 😀

[devsnek]

oh yeah i guess thats why i didn't add a context property to it. i'll have to think about this a bit more.