Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added support for JS modules and importmaps #93

Merged

Conversation

tomvanenckevort
Copy link
Contributor

Types of Changes

Prerequisites

Please make sure you can check the following two boxes:

  • I have read the CONTRIBUTING document
  • My code follows the code style of this project

Contribution Type

What types of changes does your code introduce? Put an x in all the boxes that apply:

  • Bug fix (non-breaking change which fixes an issue, please reference the issue id)
  • New feature (non-breaking change which adds functionality, make sure to open an associated issue first)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • My change requires a change to the documentation
  • I have updated the documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed

Description

As described in #92, this PR adds support for handling new ECMA JS modules and importmaps.

  • Updated JsScriptingService to also handle the module and importmap types (for <script type="module"> and <script type="importmap"> elements).
  • Updated EngineInstance to run different Jint routines based on the type passed in:
    • JS mime type: evaluate the script like before.
    • module: import the module source.
    • importmap: parse the import map, create a list of modules to import based on document path and then import them.
  • Added JsModuleLoader to use the ResourceLoader to fetch any external scripts imported from inside another module.
  • Added unit tests to test the new feature.

Note: to enable the deserializing of the importmap JSON I added a reference to System.Text.Json to the project, which would become an additional dependency of the NuGet package. I don't see any issues with that, but let me know if you think otherwise.

Copy link
Contributor

@FlorianRappl FlorianRappl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is good - we should just see if we can get rid of the dependency on system.text.json. Rest looks great to me.

Also there are some cases that potentially require a bit more code, but we can also cover those later (e.g., that paths are mapped correctly and that non-ESM vs ESM behaves as it should).

Also the scopes for importmodules are imho not correctly in there - but I think this will be difficult if Jint does not support dynamic resolution.

src/AngleSharp.Js/AngleSharp.Js.csproj Outdated Show resolved Hide resolved
@tomvanenckevort
Copy link
Contributor Author

Also the scopes for importmodules are imho not correctly in there - but I think this will be difficult if Jint does not support dynamic resolution.

Could you elaborate a bit more on that?

@FlorianRappl
Copy link
Contributor

Could you elaborate a bit more on that?

Yeah so what I meant is that the scope field gives ESM modules different parts, e.g.:

  • general imports, "react" -> "react1.mjs"
  • specific one scope to "/foo/*" -> "react" -> "react2.mjs"

Now starting with some imports each one would resolve "react" to "react1.mjs", while an import of a module "foo/something.mjs" would find "react2.mjs" when importing "react". This, however, cannot statically be set, but only when resolving the module (i.e., when Jint would find an "import" statement it would go to the dynamic resolver with the current module's metadata - most importantly its URL - to find what module is actually meant / requested here).

The scope's are not dependent on the document's path (surely, in a way they are - as this is the base path for all relative paths, but once you import from https://esm.sh/... other modules are also imported relative to this one).

@tomvanenckevort
Copy link
Contributor Author

I've refactored the importmap scopes to work as intended (based on the path of the importing script, rather than the path of the document).

@FlorianRappl
Copy link
Contributor

I've refactored the importmap scopes to work as intended (based on the path of the importing script, rather than the path of the document).

Awesome - thanks!

The only thing that keeps me at the moment from merging is that the Linux build (or rather the tests) is failing. I did not have yet the chance to look into it, but it should definitely also work on Linux.

@tomvanenckevort
Copy link
Contributor Author

Yes, was going to try and have a look at that one today as well.

@FlorianRappl
Copy link
Contributor

You are the man! GJ! 🚀

@FlorianRappl FlorianRappl merged commit 4ecbd42 into AngleSharp:devel Mar 18, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants