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

LibWeb+LibJS: Implement support for module scripts #15275

Conversation

networkException
Copy link
Member

This pull request is work towards getting websites using <script type="module"> to work properly

Copy link
Member

@davidot davidot left a comment

Choose a reason for hiding this comment

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

Just posting one comment to make sure I don't forget about it

Userland/Libraries/LibJS/AST.cpp Outdated Show resolved Hide resolved
Userland/Libraries/LibWeb/HTML/Scripting/Script.h Outdated Show resolved Hide resolved
@davidot
Copy link
Member

davidot commented Sep 18, 2022

Also for reference this PR uses/implements inspired by: whatwg/html#8264

And potentially https://github.com/nicolo-ribaudo/modules-import-hooks-refactor

@networkException networkException force-pushed the module-here-module-there-module-everywhere branch 2 times, most recently from 83be47b to 1e8a758 Compare September 18, 2022 21:36
@networkException networkException force-pushed the module-here-module-there-module-everywhere branch from 1e8a758 to 0244a5b Compare September 21, 2022 19:31
@networkException
Copy link
Member Author

I'll clean up more of the factoring and commit history in a bit. Currently this is mostly blocked on HostResolveImportedModule crashing because the current realm doesn't have [[HostDefined]] set (we need current settings object) and I don't know what we are missing to properly set [[HostDefined]]

@networkException networkException force-pushed the module-here-module-there-module-everywhere branch 5 times, most recently from c58dee8 to 2eb7597 Compare October 3, 2022 01:07
@networkException
Copy link
Member Author

networkException commented Oct 3, 2022

As of the commits I just pushed https://nwex.de/xhr.html started working :^)

@networkException networkException force-pushed the module-here-module-there-module-everywhere branch 4 times, most recently from 74240ab to b788946 Compare October 4, 2022 15:53
@networkException networkException marked this pull request as ready for review October 4, 2022 17:19
@networkException networkException changed the title LibWeb+LibJS: Implement support for module scripts (wip) LibWeb+LibJS: Implement support for module scripts Oct 4, 2022
@networkException
Copy link
Member Author

Oh I wanted to have a proper solution to the realm execution context issue before marking this as ready. I'll look into that in a bit

@networkException networkException force-pushed the module-here-module-there-module-everywhere branch 2 times, most recently from 9057031 to f1eb2db Compare October 4, 2022 19:17
Copy link
Member

@davidot davidot left a comment

Choose a reason for hiding this comment

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

Just some initial comments.
I left a couple of comments about errors and only later saw we do crash on those. So since this is kind of an initial attempt we could ignore them for now.

I can/will take a deeper look if you feel it's ready

@networkException
Copy link
Member Author

I left a couple of comments about errors and only later saw we do crash on those.

My main focus for this first cut was my own website but I can make more code paths work if you have a test case

@networkException networkException force-pushed the module-here-module-there-module-everywhere branch 3 times, most recently from a151e19 to 6b38de2 Compare October 5, 2022 21:49
@davidot
Copy link
Member

davidot commented Oct 5, 2022

I left a couple of comments about errors and only later saw we do crash on those.

My main focus for this first cut was my own website but I can make more code paths work if you have a test case

Yeah I didn't notice the VERIFYs/TODOs you hit later on in the parse fail case.
It's fine to go ahead and get this PR in and just expand on that later.

Copy link
Member

@davidot davidot left a comment

Choose a reason for hiding this comment

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

Once again just want to say great work!

Not all cases are covered but as a first step this feel good enough for me.
Certainly with the rewrite of the module spec (once from whatwg and another incoming from ecma) this will have to be looked at later as well.

networkException and others added 12 commits October 6, 2022 16:22
Previously we would simply check the an input string against a list of
mime type essences, ignoring that the input might not be a valid mime
type or contain parameters.

This patch moves the helpers into the MimeSniff namespace and properly
parses an input string before comparing the essence.
This patch adds the module type allowed steps given a module type string
and an environment settings object.
This patch adds the [[HostDefined]] field defined in
https://tc39.es/ecma262/#table-module-record-fields to module records.

Co-authored-by: davidot <[email protected]>
This patch adds support for all child classes of Web::HTML::Script to be
used in the [[HostDefined]] field of JS::Modules and JS::Scripts.
This patch adds the ModuleMap class used to keep track of the type and
url of a module as well as the fetching state associated. Each
environment settings object now also has a module map.
This patchs adds the Web::HTML::Script subclass ModuleScript and
JavaScriptModuleScript as a type of ModuleScript as well as various
algorithms related to JavaScript module scripts.

Co-authored-by: davidot <[email protected]>
This patch adds various algorithms required to fetch and link module
scripts.

Some parts such as actually creating a request and error handling are
not implemented or use temporary non spec compliant code to get us
further.

Co-authored-by: davidot <[email protected]>
This patch adds support for the HostGetSupportedImportAssertions and
HostResolveImportedModule host hooks.

Co-authored-by: davidot <[email protected]>
This patch adds support for script elements with the type attribute set
to "module". As a first cut the changes are mainly focused around inline
scripts.

Co-authored-by: davidot <[email protected]>
This patch adds a non standard step pushing the realm execution context
of fetching client's settings object onto the execution context stack
before linking a module script. Without the realm execution context
there is no current settings object, leading to a crash in
HostResolveImportedModule.
@networkException networkException force-pushed the module-here-module-there-module-everywhere branch from 6b38de2 to f8d7c33 Compare October 6, 2022 14:30
@awesomekling awesomekling merged commit a182bc9 into SerenityOS:master Oct 6, 2022
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.

4 participants