-
-
Notifications
You must be signed in to change notification settings - Fork 413
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
Implement module execution #2922
Conversation
Test262 conformance changes
Fixed tests (154):
Broken tests (8):
|
By the way, the test count reduction is because we were running module tests twice: on no strict and strict mode. However, this is unnecessary for modules because they're strict by default, so we can just run them once. |
Ok, I checked the regressed tests and all of them have the |
Codecov Report
@@ Coverage Diff @@
## main #2922 +/- ##
==========================================
- Coverage 52.15% 50.45% -1.70%
==========================================
Files 436 442 +6
Lines 43777 45267 +1490
==========================================
+ Hits 22831 22840 +9
- Misses 20946 22427 +1481
|
All documented and added an example, this is ready for full reviews now! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still reviewing the execution parts. The ast and parser changes look very good, I just found a few copy-pasted docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some small nitpicks, the rest of the code looks great!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just very awesome work on this @jedel1043. I checked out #2932 locally and it seems like with these huge steps we are on a very good path for getting most of the language features done.
I also like the optimizations built into the module environment bindings and the safe design on the Status
transitions.
I'm all for merging this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks perfect to me! Great work!
Depends on #2921. Will rebase ontomain
when that PR is merged.This Pull Request closes #64. We finally have a module system! 🎉
It changes the following:
Module
struct API from which an user can progress the loading, linking and execution of a module.Context
's module APIs in favour of methods inModule
.ModuleLoader
trait from where a host can define how to load and cache modules.ModuleEnvironment
that can contain indirect references to other environment bindings.There's still more work to do though, like adding support for the
import()
method, fix some parsing and linking bugs, and implementing JSON modules (Stage 3).Marking it as draft since there's a lot of documentation missing
(and apparently some bugs), but I wanted to put out the PR for reviews on the implementation.