Closed rhuanjl closed 3 years ago
@liminzhu what do you think of this?
Thanks for taking a look at this @rhuanjl! Your reasoning seems sound to me, and I agree that trying to push our square peg of module loading into node's round hole is a lot of work as things stand.
The only thing I'm wary of is just how much we want to change chakracore to make it more v8-like. I'm not well versed in the module loading side of things (@boingoing might be a better person to take a look at the code you wrote?) but from what I saw your change looked reasonable from a technical standpoint.
Thanks for the comments @MSLaguana I understand not wanting to make CC more like v8 - in particular for this case as I note above I think the existing CC module API is nicer than v8s.
It's just that for node-chakracore shimming v8's behaviour on top of the existing CC API will be beyond difficult - hence the proposal here of implementing this secondary module loading pathway/extra APIs basically intended for node-chrakracore only (though obviously available if someone else particularly wants to use them).
Note also I wouldn't expect those commits to be merged until there's a working node-chakracore implementation using them - I just didn't want to go further with creating that until I had some feedback on the idea (also I've now noted a couple of places the logic could be simpler but I'll hold off on updating for now)
// cc @digitalinfinity
Appreciate the writeup @rhuanjl !
I'm similarly uninformed on how v8 module API works but I assume once we start working on module support for Node-ChakraCore someone will take a look at V8 APIs and form an educated opinion on the matter (I'd love to weigh in once I got some cycles to study how v8 and node work as well). For now, I share similar concern as @MSLaguana .
One extra thing I'd like to bring up is there's discussion within TC39 and WebAssembly CG about wasm and es modules interop. The outcome isn't clear yet, but wasm module interoperability could potentially affect how we want to expose module APIs. Something to think about.
@liminzhu thanks for the comments couple of things to note:
I don't think wasm interop should have any significant impact on these sorts of changes - as this is to do with controlling the list of modules you need to load and the order you load not really how you load them.
To be clear I was/am offering to make a PR to implement ESModules in node-chakracore if this suggestion is accepted - it wouldn't take me that long.
To be clearer on motivation for this sort of change - I genuinely don't think it will be possible to support ESModules in node-chakracore without a change of this sort - unless both node's internal code in this area and the exposed vm.Module class in node can be re-written extensively - I spent quite a lot of time looking at it and comparing v8's and CC's APIs before thinking this was a good suggestion.
Anyhow no rush - just trying to offer something helpful.
Oh thanks for offering to help! I will spend some time looking into it. Maybe @boingoing and some other folks will join in as well.
Note, I have a working implementation/proof of concept of this proposal that passes all existing module tests. See this commit for CC changes https://github.com/rhuanjl/ChakraCore/commit/db4daf7b1e21c952c6bd256291f987f0fd428069 and See this commit for adding it to ch https://github.com/rhuanjl/ChakraCore/commit/cb316f62575ed02920bc774cc87663d3daa6790a
Background I spent some time looking at ways of implementing ESModules in node-chakracore but noted that:
Therefore CC needs a revised module API for node ESModule support to become a thing (unless a wrapper could be written around the v8 api instead - you could shim CC's API over v8's relatively easily I think)
However - from having worked with it I think that CC's API is a far nicer API than v8's so for anyone else embedding CC and using ESModules I think it would be a shame if CC's API had to be changed to reflect v8's on this.
Proposal
Detailed Idea for alternate pathway
Using these APIs a lot of the work that would be done inside CC with the existing API has to be done by the host most notably:
But this is exactly what v8's API requires and hence what node already does.
Other notes Currently you need to know if a module is a root module or not when you first create it which I don't think v8 does - I think I can change this with a little more tweaking, will fix this point if there's interest in this proposal.
I haven't yet got a version of node-chakracore working with this - but if there's interest in this proposal I should be able to do that relatively easily as this alternate API basically mirror's v8's so should be able to offer a PR to implement that in 2-3 weeks.