Support ECMAScript modules (export/import statements, modules definition) #1054

christianrondeau · 2022-01-17T15:39:45Z

This attempts to implement ECMAScript modules, to resolve #636. This is a draft and will definitely change, but as of now, I think the basic "export named" flow is mostly correct.

What I want to implement:

I do not intend on implementing module file resolution (only modules specified in C#) for this PR, but I feel like there may not be much left to do once this works.

Note that I did (and will) use code from @lahma #1051 - the main difference is that I started with the assumption that I would only implement export, however, to make sure my implementation is correct, I need to implement at least one simple import statement.

Jint/Engine.Modules.cs

sebastienros · 2022-01-18T21:38:33Z

Was wondering why Christian's name was so familiar. It's because he's been active on Jint since 2015, way before Marko.

christianrondeau · 2022-01-19T17:31:42Z

@sebastienros yeah it's been a while, I was glad to see that Jint was still alive and well!

So, I went much further than I initially planned to, but I felt that without all those pieces interacting together, I wouldn't be able to trust that my approach made sense (and indeed, by adding pieces I realized a few times I approached it wrong!)

There are a lot of things in this PR, so while I think a good review would be important, I'll try and raise some specific uncertainties I had myself so we can get them out of the way. I'll use the GitHub code comments to do so, I'll write a message when I reviewed it all myself at least once. After those are resolved, we can proceed with an actual review.

I don't think I completely mapped to the ECMAScript specifications, but to be honest I have trouble fully understanding it. I'll try and review it the best I can and raise questions if I'm not sure.

There are also a few things that are Jint-specific:

The ability to create a module in C#, e.g. engine.DefineModule("export const x = 1;", "my-module")
The ability to create a module from values directly, e.g. engine.DefineModule<MyClass>() or `engine.DefineModule("exportName", JsString.Create("Hello"), "my-module");
The ability to import a module from C#, e.g. var ns = engine.ImportModule("specified") which allows reading the exports in C#
I added a new DefaultModuleResolver, which is responsible for resolving paths and/or bare specifiers. This is a small breaking changes since now the DefaultModuleLoader is not responsible for doing the resolution, and receives a ResolvedSpecifier. This was necessary in order to have modules ./my-module and ../folder/my-module reference the same module instance.

I hope this PR will see the light of day and that I didn't do anything dumb :)

christianrondeau · 2022-01-19T17:34:44Z

The tests fails because System.IO.DirectoryNotFoundException : Could not find a part of the path 'C:\Users\runneradmin\AppData\Local\Temp\2a69ed27-1218-4ccb-b5d7-45e8321e1e0a\2a69ed27-1218-4ccb-b5d7-45e8321e1e0a\assembly\Runtime\Scripts\modules\format-name.js'. - it seems the way I find the scripts folder is incorrect, I'll get back to this too later. I'm not sure why the Linux tests fail though.

christianrondeau · 2022-01-20T05:28:17Z

I'm done with my first review; I added comments and questions on things that I'm not sure about. It's a pretty big PR for someone with limited knowledge of ASTs, language specifications etc but I feel like the vast majority of the code required was already written. I'm looking forward to hearing your thoughts on this!

lahma

Looking good! As you can see most of my comments were stylistic nit-picking. The import/export API via c# engine still doesn't seem intuitive to me, but I'm just one person. You really went the extra mile with this.

I don't have good guidance whether this is spec-wise "perfect", when I read the relevant parts it seemed that there were some places where "magic just happens", so I'm happy we have an interpretation of behavior that passes tests!

lahma · 2022-01-17T16:27:11Z

Jint.Tests/Runtime/ModuleTests.cs

@@ -0,0 +1,42 @@
+using Xunit;
+
+namespace Jint.Tests.Runtime


can use file-scoped namespaces here, we are moving towards them

Maybe this also belongs to Module folder that the next test file has?

lahma · 2022-01-17T16:28:17Z

Jint.Tests/Runtime/ModuleTests.cs

+        [Fact]
+        public void CanExportNamed()
+        {
+            _engine.DefineModule(@"export const value = 'exported value';", "my-module");


DefineModule as a name is a bit different from the Execute we have for scripts, this is the first Define* as we know have Set. The method pretty much says what it's going to do though.

Here I'm just thinkin out loud how we should formulate final engine API for 3.x...

lahma · 2022-01-20T06:02:51Z

Jint.Tests/Runtime/ModuleTests.cs

+
+namespace Jint.Tests.Runtime
+{
+    public class ModuleTests


And why we need to manually test these and Test262 suite isn't helping. I did investigate a bit and seems that some module related tests light up only when async/await is implemented. So it's great that you created such extensive test cases!

lahma · 2022-01-20T06:11:13Z

Jint.Tests/Runtime/ModuleTests.cs

+        [Fact]
+        public void ShouldDefineModuleFromJsValue()
+        {
+            _engine.DefineModule("value", JsString.Create("hello world"), "my-module");


This API doesn't seem intuitive to me. Single value cannot be a module so here's something else going on here...

// export maybe part of the name? engine.AddModuleExport("value", JsString.Create("hello world"), "my-module") engine.Modules.AddExport("value", JsString.Create("hello world"), "my-module") // maybe goes too far.. engine.Advanced.Modules.AddExport("value", JsString.Create("hello world"), "my-module")

Just throwing ideas here. The overload taking just object might bee too much also. Without it one can write:

_engine.DefineModule("value", "hello world", "my-module"); _engine.DefineModule("value", 123, "my-module");

If such functionality is needed I think it would be better to force API user to do the call JsValue.FromObject()?

These are advanced use cases I believe allowing cool stuff but not entirely necessary to be able to run stock ES6 code.

lahma · 2022-01-20T06:15:44Z

Jint.Tests/Runtime/ModuleTests.cs

+        [Fact]
+        public void ShouldDefineModuleFromClrType()
+        {
+            _engine.DefineModule<ImportedClass>("imported-module");


This doesn't seem intuitive either. Module can export a class for sure, but it's not a module.

I see that the code ends into export path and this is somehow an imported module 🤷🏻‍♂️ So is the API for registering Import or an Export...

Jint/Runtime/Modules/ModuleResolutionException.cs

lahma · 2022-01-20T06:54:24Z

Jint/Runtime/Modules/JsModule.cs

@@ -2,6 +2,7 @@
 using Esprima.Ast;


#nullable enable?

lahma · 2022-01-20T06:54:40Z

Jint/Runtime/Modules/ModuleResolutionException.cs

@@ -0,0 +1,11 @@
+using System;


#nullable enable?

lahma · 2022-01-20T06:55:21Z

Jint/Runtime/Interpreter/Statements/JintImportDeclaration.cs

@@ -0,0 +1,28 @@
+using System;


#nullable enable?

Jint/Engine.Modules.cs

sebastienros

Some of my comments my collide with @lahma 's around naming.

sebastienros · 2022-01-20T15:23:55Z

Jint.Tests/Runtime/ModuleTests.cs

+        public void ShouldExportNamed()
+        {
+            _engine.DefineModule(@"export const value = 'exported value';", "my-module");
+            var ns = _engine.ImportModule("my-module");


Does ImportModule actually import a module in the environment, or does it just return it to the caller, and doing an Execute('return value') would not work.

Jint.Tests/Runtime/ModuleTests.cs

Jint/Engine.Modules.cs

sebastienros · 2022-01-20T15:34:52Z

Jint/Native/Function/ScriptFunctionInstance.cs

@@ -138,7 +138,7 @@ ObjectInstance IConstructor.Construct(JsValue[] arguments, JsValue newTarget)
            {
                try
                {
-                    var result = OrdinaryCallEvaluateBody(_engine._activeEvaluationContext, arguments, calleeContext);
+                    var result = OrdinaryCallEvaluateBody(_engine._activeEvaluationContext ?? new EvaluationContext(_engine), arguments, calleeContext);


I agree, this needs to be found. Please share the stacktrace when this happens.

Jint/Native/Object/ObjectInstance.cs

sebastienros · 2022-01-20T15:39:52Z

Jint.Tests/Runtime/ModuleTests.cs

+            engine.DefineModule("import { User } from './modules/user.js'; export const user = new User('John', 'Doe');", "my-module");
+            var ns = engine.ImportModule("my-module");
+
+            Assert.Equal("John Doe", ns["user"].AsObject()["name"].AsString());


Isn't it sufficient to let users access the module properties from the engine since the module was loaded? engine.GetValue("user") just to reduce the amounts of ways to do something, which could lead to inconsistencies, or discrepancies in behavior. So ImportModule would return nothing. (or Engine)

Jint/Engine.Modules.cs

christianrondeau · 2022-01-20T21:21:22Z

I will now add the ModuleBuilder idea that came up a few times in the reviews, I think it will slightly reduce the impact on Engine (remove DefineModule variants) and refactor the tests accordingly. The idea is to do something like this:

var module = new ModuleBuilder();
module.ExportClass<T>("name");
module.ExportValue("name", JsValue);
engine.LoadModule(module, "module-name");

If you disagree it's now or never :D In the future, that should also allow us to do things like parsing JavaScript expressions, create multiple bindings, etc.:

module.CreateImmutableBinding("varName", JsValue);
module.AddExpression("const test = varName + 1;");
module.AddExpression("import { stuff } from 'other-module';);
module.ExportBinding("test", "exportName1");

christianrondeau · 2022-01-21T01:47:17Z

All right I think I have something I'm happy with that solves some of the comments. Here's how you can define modules now:

Method 1, using source code. Nothing new here, except the rename:

engine.AddModule("module-name", "export const x = 1;");

Method 2, using the builder. This allows you to build a module with code, objects, types, etc.

engine.AddModule("module-name", builder => builder
  .AddSource("export const value1 = 'hello';")
  .ExportType<MyType>()
  .ExportValue("value2", "world")
  .ExportObject("value3", new MyType())
  .ExportFunction("fn", args => Console.WriteLine(args.Length))
);

This is, I think, fairly discoverable (there's only AddModule there, and only two overloads for it), it allows building modules from multiple pieces of data, it can be extended later, and doesn't need to interact with engine twice.

Also, bonus, it really lazy loads modules now, so even custom-built modules won't be linked until they are loaded, making it much better if you wanted to offer a large set of modules to optionally import.

I feel like actually supporting node_modules wouldn't be a stretch, but I'm already way out of my initial need so shush me :D

Once a module has been added, you can use engine.ImportModule("module-name") to "run" the program. Note that there could be an argument for RunModule or ExecuteModule, but the reason I still stand behind Import is because you're actually acting like the parent module in the C# code. But I can live with ExecuteModule, as long as it still returns the module namespace.

In other words, I see two ways to use it:

Like Node.js, e.g. node file.js where you run whatever's in file.js and don't really deal with "returned values" (considering stdout isn't really "returning"). This is where engine.ExecuteModule() makes the most sense.
As the parent script, e.g. var userDefinedLogic = engine.ImportModule() where you want to invoke the user's code, and/or read output values, and need output.

Since ImportModule actually executes the module, I don't think we need both but we could have ExecuteModule() call ImportModule() and drop the result if you want.

christianrondeau · 2022-01-24T14:37:14Z

Two quick questions.

~~Do you want me to rebase or should we wait for all reviews/feedbacks to be completed?~~ I just did it
I was thinking of @sebastienros feedback on wanting to use "Execute" to run modules. While I still stand by my opinion that "modules should be separated from scripts", I thought of a few improvements to AddModule that I'd like to share. They won't change any of the existing code so it shouldn't invalidate any of the remaining reviews.

Right now to "execute" a module you need to "import" it in the C# context:

engine.AddModule("module-name", "export const x = 1;");
var ns = engine.ImportModule("module-name");
Console.WriteLine(ns.Get("x").AsString());

This is the equivalent of:

echo "export const x = 1;" > module-name.js
echo "import { x } from 'module-name.js'; console.log(x);" > run.js
node run.js

This is why it's confusing, we're actually creating AND running modules from the same context. However to create and run modules will probably be a pretty common pattern. So to simplify it, I had two new "sugar" APIs in mind.

Fluent API to "Add and Import"

This keeps the same proposed semantics.

var ns = engine.AddModule("module-name", "export const x = 1;").ImportModule();
Console.WriteLine(ns.Get("x").AsString());

Sugar API to "AddAndImport":

This adds a new word, "Execute" which here means "Add And Import". It also hides the module name, making it more in line with what you expect from Execute. However it does not add any of the lexical context into the "normal" Execute" method. The module name would be automatically generated (e.g. a GUID or something like "dynamic")

var ns = engine.ExecuteModule("export const x = 1;");
Console.WriteLine(ns.Get("x").AsString());

Let me know what you think :)

Addendum: engine.ExecuteModule(script) has an issue IMHO, since I'd expect to be able to do engine.ExecuteModule(moduleName), both have strings as their inputs and it's impossible to know which of the two will happen unless you look at the docs or try. Instead, we could support engine.ExecuteModule(builder => builder.AddSource("...")) and engine.ExecuteModule(moduleName). That would be more in line with the eventual node_modules support. Another thought is for the equivalent of npm run (something) that will eventually end up in the API, potentially causing more confusion around Run v.s. Execute. With that thought process, Execute in the context of modules would simply not exist, and only Run (shell scripts using node as the hashbang) or Import (reference a module and do stuff with it) would remain.
Long story short, while it looks good at first glance, I'm not sure ExecuteModule is a great choice. The "Fluent API" would be cheap to add if you like it though.

christianrondeau · 2022-02-01T15:33:14Z

I may have greatly reduced availability soon; while I won't push for a merge (I'm completely aware of what maintaining an open-source project means) I'd like to know if you believe this will eventually get merged so that I can start building on top of it. If you're not sure yet, fair enough :) If it's just a matter of taking the time to fully review but you feel that the approach is sound, I'll temporarily work on this branch for my downstream projects.

Thanks :)

sebastienros · 2022-02-01T16:04:44Z

Jint/Runtime/Modules/DefaultModuleLoader.cs

    }

-    protected virtual string LoadModuleSourceCode(Uri location)
+    protected virtual string ReadAllText(Uri uri)


This is not reusable anymore in a sub-class. The fact that the checks are done outside will prevent custom implementation from loading urls.

We'll need this one fixed before merging. I will do it otherwise.

sebastienros · 2022-02-01T16:07:33Z

I added only one comment, but would like to see @lahma 's answered too.
When this is done I will merge it.

lahma · 2022-02-12T14:37:22Z

@christianrondeau can we help out here somehow? would be nice to get this finalized and merged 🚀

…unction

christianrondeau · 2022-02-12T19:59:19Z

@lahma as far as I know, all of your and @sebastienros comments have been solved or I had a follow-up question (see https://github.com/sebastienros/jint/pull/1054/files) but on my end, I'm comfortable with merging in its current state. I just rebased, all tests were green :) If I missed one question let me know, I'm also eager to see this merged! Also I had some proposals that were unanswered but those can always be improvements in future PRs, since I believe it'll either be naming changes or sugar, but the general idea should be stable.

lahma · 2022-02-15T16:10:29Z

I checked the PR and there are some comments, can you mark resolved the ones are non-actionable etc?

christianrondeau · 2022-02-16T15:08:33Z

@lahma I'm not allowed to mark comments as resolved :) I did go through all comments last time, most of them are still unanswered. I added a comment on as many as I could to mark the ones I think are not actionable, and deleted my own unanswered questions to reduce the scope. I don't think I can do anything more at this point (but I'm willing to!).

lahma · 2022-02-16T16:50:33Z

I hope I'm not the broken phone here, current state is that it was mostly nit-picking and we can always fix/break things having the beta label. Pinging @sebastienros on the matter.

christianrondeau · 2022-02-16T17:01:39Z

Haha by default I always take for granted that I'm the one missing something until proven wrong (or right) ;)

Here's what I am looking at:

https://github.com/sebastienros/jint/pull/1054/files scrolled through all comments
Support ECMAScript modules (export/import statements, modules definition) #1054 (comment) I proposed something asking for feedback that I ended up implementing to solve some of the comments about terminology
https://github.com/sebastienros/jint/pull/1054/files#r796750859 specifically is the only one with instructions I did not implement because it would break something else, I asked whether to retract the comment or proposed an alternative.
(edit) There was also a comment by @sebastienros saying "how is that resolved?" but it's on a file that was since removed, I couldn't find what it was about from the history nor from the highlighted code. It was on the namespace statement of DefaultModuleResolverTests.cs.

But I reiterate I want to make things as simple as I can for you, if you see what I'm missing I'll do my best to fix it.

christianrondeau · 2022-02-22T00:06:57Z

(in case it wasn't clear, the list I posted shows everything that I can see but I have no todos on my end as far as I can tell, I'm on hold)

lahma · 2022-03-03T19:51:26Z

@christianrondeau awesome work! thank you for pushing this through and being patient with us!

christianrondeau · 2022-03-03T23:47:52Z

Woot! Thanks @lahma and @sebastienros ! Since I hope to contribute again, do you have any feedback on this PR? I know it was too large (I didn't find a way to reliably test the import without export and vice versa) but otherwise, is there something I could have done to make it easier for you guys? I feel like throwing tons of questions directly in the file changes might not have been the best approach :)

lahma · 2022-03-04T07:06:52Z

I think the process was mostly long due to the feature being both large and had no specific spec to rely on at times (implementation specific). Hard to predict all the use cases and how people would like to interact with it "outside of JS".

Feedback loop is always a bit of a problem when people try to find some free time (which is taken away from something else) and are also on different time zones 🙂

lahma · 2022-03-04T09:18:40Z

I'll try to do a quick round with more seamless test suite integration and allowing people to run scripts with imports without jumping many hoops. I'll create a separate PR for feedback.

christianrondeau · 2022-03-04T12:05:11Z

@lahma I didn't mean that it wasn't fast enough, no worries!!! I just wanted to make sure my next PR is painless for you guys :) Thanks for helping me through!

christianrondeau changed the title ~~Export named declaration (#636)~~ Support ECMAScript modules (export/import statements, modules definition) Jan 17, 2022

christianrondeau force-pushed the export branch 2 times, most recently from 996377d to c4aae01 Compare January 18, 2022 02:01

sebastienros reviewed Jan 18, 2022

View reviewed changes

Jint/Engine.Modules.cs Outdated Show resolved Hide resolved

christianrondeau force-pushed the export branch from 084a055 to 38de185 Compare January 19, 2022 04:22

lahma marked this pull request as ready for review January 19, 2022 18:09

lahma marked this pull request as draft January 19, 2022 18:10

christianrondeau force-pushed the export branch from cc3cf31 to 0e0fb5a Compare January 20, 2022 04:54

lahma reviewed Jan 20, 2022

View reviewed changes

sebastienros reviewed Jan 20, 2022

View reviewed changes

Jint/Engine.Modules.cs Show resolved Hide resolved

christianrondeau force-pushed the export branch from 2e115d9 to 9580f04 Compare January 21, 2022 01:50

christianrondeau marked this pull request as ready for review January 21, 2022 15:10

christianrondeau force-pushed the export branch from 928d552 to 6309748 Compare January 27, 2022 17:08

sebastienros reviewed Feb 1, 2022

View reviewed changes

christianrondeau added 5 commits February 12, 2022 13:25

Export named declarations (sebastienros#636)

94649e9

Import named declaration (reused code from @lahma)

ac94434

Correctly propagate throw through module imports

219a74e

Run available continuations after module import

7f996b2

Export default

7f273eb

christianrondeau added 6 commits February 12, 2022 13:30

Build modules using AddModule and ModuleBuilder

66f30b7

Unload ModuleBuilder from memory after loading, ModuleBuilder.ExportF…

95ce1b3

…unction

Add test case for invoking imported user defined classes

9aae8fe

Simplify ModuleTests Invoke by using the right APIs

96c6f43

Add documentation for modules

81ddf38

Fix Linux build and improve resolution tests

2351c37

christianrondeau force-pushed the export branch from 6309748 to 2351c37 Compare February 12, 2022 19:56

sebastienros added 8 commits March 3, 2022 07:16

Update ModuleTests.cs

ef4427c

Update DefaultModuleResolverTests.cs

1e74771

Missing new line

d32422e

Missing new line

ef3563b

Rename options

4653d7a

Test c# 10

9e6db8d

DefaultModuleLoader is not extensible

0600c09

Merge branch 'main' into export

0bcff4e

sebastienros enabled auto-merge (squash) March 3, 2022 16:50

sebastienros merged commit fcc7c8d into sebastienros:main Mar 3, 2022

christianrondeau deleted the export branch March 3, 2022 23:48

christianrondeau mentioned this pull request Mar 6, 2022

Module handling fixes and improvements #1097

Closed

Support ECMAScript modules (export/import statements, modules definition) #1054

Support ECMAScript modules (export/import statements, modules definition) #1054

Conversation

christianrondeau commented Jan 17, 2022 • edited

sebastienros commented Jan 18, 2022

christianrondeau commented Jan 19, 2022

christianrondeau commented Jan 19, 2022

christianrondeau commented Jan 20, 2022

lahma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lahma Jan 20, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebastienros left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christianrondeau commented Jan 20, 2022 • edited

christianrondeau commented Jan 21, 2022 • edited

christianrondeau commented Jan 24, 2022 • edited

christianrondeau commented Feb 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebastienros commented Feb 1, 2022

lahma commented Feb 12, 2022

christianrondeau commented Feb 12, 2022 • edited

lahma commented Feb 15, 2022

christianrondeau commented Feb 16, 2022

lahma commented Feb 16, 2022

christianrondeau commented Feb 16, 2022 • edited

christianrondeau commented Feb 22, 2022

lahma commented Mar 3, 2022

christianrondeau commented Mar 3, 2022

lahma commented Mar 4, 2022

lahma commented Mar 4, 2022

christianrondeau commented Mar 4, 2022

christianrondeau commented Jan 17, 2022 •

edited

lahma Jan 20, 2022 •

edited

christianrondeau commented Jan 20, 2022 •

edited

christianrondeau commented Jan 21, 2022 •

edited

christianrondeau commented Jan 24, 2022 •

edited

christianrondeau commented Feb 12, 2022 •

edited

christianrondeau commented Feb 16, 2022 •

edited