Module loading support #990
Conversation
|
Would you like some initial code review already or at later stage,? |
|
@lahma some initial feedback is always welcome :) |
|
How close did you follow the spec? Update |
|
Great effort. I wish you had added a simple example to show it works. Unless it doesn't ;) |
Jint/Runtime/Modules/JsModule.cs
Outdated
|
|
||
| namespace Jint.Runtime.Modules | ||
| { | ||
| public sealed record ResolvedBinding(JsModule Module, string BindingName) |
There was a problem hiding this comment.
ideally make every type internal (and sealed) until someone comes up with a use case of having public types
|
@pluethi1 this is looking really good! some remarks for in-progress code, hopefully the test suite will show possible problems. |
|
@pluethi1 I hope you don't mind that I pushed some tweaks to test selection:
Only 115 failing tests of 599 so it's already pretty close. I'll look into the Esprima problems and please let us know if you want any help. EDIT: ignored |
|
So i was finally able to work on this again :) I got down to 11/599 failing tests so i'd say that's a rather big improvement. Also i refactored the code a little, based on the initial review. I decided to remove the ability to load remote modules into the engine, as it's not a real requirement in my eyes and definetly poses a security liability. I'll also prepare a little example to demonstrate how to load modules into a script context. |
|
@pluethi1 great work! I also did some research on the Esprima errors and I'm willing to mark them as skipped ( An example project would be great, maybe utilizing NPM to so it's a real-life scenario, maybe |
|
@pluethi1 can we help out here? If you are busy that's fine too. Just checking the status. |
|
@lahma I have been rather busy the last couple of weeks, so this PR got pushed back a bit on my ToDo 😅. As for your request to show an example with npm: I think supporting npm is a bit out of scope for this feature as @PeterWone mentioned on issue #430 most npm packages rely on dependencies of built in modules of node.js. Also some npm modules use the CommonJS API which is not implemented at the moment. |
Hey no rush, just wanted to check whether you had lost all hope with the Jint infrastructure 😉
It most certainly isn't, it would be nice to have somewhat good abstraction to build features upon later, but also interfaces and structures can be modified later on (we have the beta label). I think this is quite near already, just missing the green build mostly. Many of the problems were esprima related if I recall right so that's currently something that this PR cannot address unless we got some blockers fixed with the latest release (you need to rebase for that). Please let us know if we can be of help. |
|
Checked rest of Esprima related problems and this PR should address them: sebastienros/esprima-dotnet#217 |
|
I don't think it is possible to overstate the value of access to npm modules. However... The biggest problem with this is not the technical challenge of implementing node externals, but the security implications of scripts having these capabilities. To sandbox this implementations of node externals would have to expressly respect the sandbox.
There may be other things. |
|
Jint tries to be safe by default and it's also reflected in this feature, module loading is disabled by default. As a first iteration I think it's enough to warn in activation code documentation that there can be security concerns. A truly sandboxed loader can be an enhancement if someone is such willing to contribute. |
|
@pluethi1 I hope you don't mind I pushed a small cleanup/documentation commit to tweak remaining things I found / were noticed in PR review. With latest Esprima the build is now green and I guess this could be ready for review after you check that I didn't mess anything up with my commit? I'm OK if you want to revert my changes too as these were given, not asked for. |
Jint/Engine.Modules.cs
Outdated
| return module; | ||
| } | ||
|
|
||
| if (!ModuleLoader.TryLoadModule(specifier, referencingModuleLocation, out var moduleSourceCode, out var moduleLocation)) |
There was a problem hiding this comment.
What are your thoughts about returning the AST from the module loader so it could cache it. Right now it can only cache the source. Would it be beneficial, like a loader could be shared across engines, and even provide some sort of thundering-herd protection? Note that it could already do that and still return a string. Could even be a custom loader wrapper.
Today we can cache parsed programs, and create an engine from that, but returning a string means parsed modules can't be cached, so each execution will have to parse the same module again.
| GetExportEntries(true, defaultDeclaration.Declaration, exportEntries); | ||
| break; | ||
| case ExportAllDeclaration allDeclaration: | ||
| //Note: there is a pending PR for Esprima to support exporting an imported modules content as a namespace i.e. 'export * as ns from "mod"' |
| //https://tc39.es/ecma262/#sec-performpromisethen | ||
| //... | ||
| //13. If resultCapability is undefined, then | ||
| // a. Return undefined | ||
| //14. Else | ||
| // a. Return resultCapability.[[Promise]] |
There was a problem hiding this comment.
| //https://tc39.es/ecma262/#sec-performpromisethen | |
| //... | |
| //13. If resultCapability is undefined, then | |
| // a. Return undefined | |
| //14. Else | |
| // a. Return resultCapability.[[Promise]] | |
| // https://tc39.es/ecma262/#sec-performpromisethen | |
| // ... | |
| // 13. If resultCapability is undefined, then | |
| // a. Return undefined | |
| // 14. Else | |
| // a. Return resultCapability.[[Promise]] |
|
@lahma I don't mind at all! Thanks a lot for helping out :) |
Co-authored-by: Sébastien Ros <sebastienros@gmail.com>
…tration configurable
|
@sebastienros I think I went more towards your suggestions in latest commit 62351ad , so now only public |
|
Gitter is down, it let me know only when I typed the whole message ;) checking now |
I know, I wrote you a whole |
Jint/Runtime/Modules/ModuleLoader.cs
Outdated
|
|
||
| protected virtual string LoadModuleSourceCode(Uri location) | ||
| { | ||
| if (!location.IsFile) |
There was a problem hiding this comment.
Maybe should call .LocalPath and then check that it's under the defaultBasePath. For instance to block files containing ../ which would go above it.
Jint/Runtime/Modules/ModuleLoader.cs
Outdated
|
|
||
| namespace Jint.Runtime.Modules; | ||
|
|
||
| public class ModuleLoader : IModuleLoader |
There was a problem hiding this comment.
DefaultModuleLoader, because it's meant to be replaced.
Jint/Runtime/Modules/ModuleLoader.cs
Outdated
| { | ||
| private readonly Uri _defaultReferencingLocation; | ||
|
|
||
| public ModuleLoader(string defaultBasePath) |
There was a problem hiding this comment.
basePath, because there is no fallback, so "default" doesn't make much sense to me.
Jint/Runtime/Modules/ModuleLoader.cs
Outdated
|
|
||
| public class ModuleLoader : IModuleLoader | ||
| { | ||
| private readonly Uri _defaultReferencingLocation; |
There was a problem hiding this comment.
Why Uris since it only deals with files? Calling LocalPath should fold everything and then we can check for the basepath.
Please do, changes sound logical. |
I will open this PR now after @lahma encouraged me to do so 😉
Please note that this is still very much a work in progress and I won't be really able to work on it as much as I want in the coming weeks.
What's really left is implementing the tests for the module loading and optimization and eventual refactoring.
fixes #430