Implement node:vm
#2785
Implement node:vm
#2785
Conversation
src/bun.js/bindings/Script.cpp
Outdated
| { "cachedDataRejected"_s, static_cast<unsigned>(PropertyAttribute::ReadOnly|PropertyAttribute::CustomAccessor), NoIntrinsic, { HashTableValue::GetterSetterType, scriptGetCachedDataRejected, nullptr } }, | ||
| { "createCachedData"_s, static_cast<unsigned>(PropertyAttribute::ReadOnly|PropertyAttribute::Function), NoIntrinsic, { HashTableValue::NativeFunctionType, scriptCreateCachedData, 0 } }, | ||
| { "runInContext"_s, static_cast<unsigned>(PropertyAttribute::ReadOnly|PropertyAttribute::Function), NoIntrinsic, { HashTableValue::NativeFunctionType, scriptRunInContext, 0 } }, | ||
| { "sourceMapURL"_s, static_cast<unsigned>(PropertyAttribute::ReadOnly|PropertyAttribute::CustomAccessor), NoIntrinsic, { HashTableValue::GetterSetterType, scriptGetSourceMapURL, nullptr } }, |
There was a problem hiding this comment.
btw, to implement this, have a look at JSC::FunctionExecutable. I vaguely remember something in there to extract the sourceMapURL
There was a problem hiding this comment.
One question I had here was whether I should be using a custom property here or just set the field on the object during init.
I feel like the latter might be simpler, but I'm not sure exactly how to do it - if you have any examples of code that sets object fields you could link that'd be great :)
There was a problem hiding this comment.
imo we should keep it as a custom getter/setter (present state)
as an optional optimization, if we think people will frequently call it, then we could add a mutable WriteBarrier<JSC::JSString> cachedSourceMapURL; on the Script to cache it
The sourceMapURL may be lazily loaded in JSC (i haven't checked), in which case making it a property on the object would be less efficient
|
a weird thing we should consider do we want the transpiler to run in these VM global objects? if we use Zig::GlobalObject it will |
|
Interesting question. Can you think of any downsides to having it run? I guess it could have a small performance impact? |
|
yes to performance impact, but also the transpiler only outputs ESM modules which means however, does |
no $ cat vm.js
const vm = require("node:vm");
vm.runInThisContext(`require("node:fs");`);
$ node vm.js
evalmachine.<anonymous>:1
require("node:fs");
^
ReferenceError: require is not defined
at evalmachine.<anonymous>:1:1
at Script.runInThisContext (node:vm:129:12)
at Object.runInThisContext (node:vm:313:38)
at Object.<anonymous> (/home/silver/code/play-js/vm.js:2:4)
at Module._compile (node:internal/modules/cjs/loader:1165:14)
at Object.Module._extensions..js (node:internal/modules/cjs/loader:1219:10)
at Module.load (node:internal/modules/cjs/loader:1043:32)
at Function.Module._load (node:internal/modules/cjs/loader:878:12)
at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:81:12)
at node:internal/main/run_main_module:22:47 |
| } | ||
|
|
||
| ArgList args(callFrame); | ||
| return runInContext(globalObject, script, globalObject->globalThis(), globalObject->globalScope(), args.at(0)); |
There was a problem hiding this comment.
We should reuse JSC's code cache like what JSC::Eval does
| } | ||
| JSObject* context = asObject(contextArg); | ||
|
|
||
| JSScope* contextScope = JSWithScope::create(vm, globalObject, globalObject->globalScope(), context); |
There was a problem hiding this comment.
I think we have to use a new JSGlobalObject since if you assign to a global in a context it shouldn't impact the calling context
I think we can use JSProxy here:
WebCore has to do some similar strange things due to the combo of:
- Web Extensions
- iframes accessing parent window
- regular userland code
|
how about screwing the cjs syntax and only supporting the ESM system as nodes experimental vm? |
The goal for node:vm in bun is to improve node compatibility, so no |
also oops I forgot to commit the new files last time
|
Not complete but the rest is beyond my JSC knowledge right now so this can be merged as a first step :) Hopefully it should get a decent chunk of the stuff that depends on |
|
great |
Resolves #401