WIP: Implement Import assertions and JSON modules#1039
WIP: Implement Import assertions and JSON modules#1039LowByteFox wants to merge 5 commits intoquickjs-ng:masterfrom LowByteFox:import_assertions
Conversation
bnoordhuis
left a comment
bnoordhuis
left a comment
There was a problem hiding this comment.
Only a partial review because it's late and I need to get up early tomorrow. Can you apply this diff and run make test262?
diff --git a/test262.conf b/test262.conf
index a59fe3d..fc16a11 100644
--- a/test262.conf
+++ b/test262.conf
@@ -112,7 +112,7 @@ generators
globalThis
hashbang
host-gc-required=skip
-import-assertions=skip
+import-assertions
import-attributes=skip
import-defer=skip
import.meta|
|
||
| /* To minimize creating breaking changes, I have instead decided | ||
| to carry the parsed import_assertion instead */ | ||
| JSValue import_assertion; |
There was a problem hiding this comment.
Mark/free this in JS_MarkContext & JS_FreeContext.
| JSValue meta_obj; /* for import.meta */ | ||
|
|
||
| /* a list of key/value strings - [key, value, key, value] */ | ||
| JSValue import_assertion; |
There was a problem hiding this comment.
Mark/free this in js_mark_module_def & js_free_module_def.
|
|
||
| JSValue JS_GetImportAssertion(JSContext *ctx) | ||
| { | ||
| return ctx->import_assertion; |
There was a problem hiding this comment.
| return ctx->import_assertion; | |
| return js_dup(ctx->import_assertion); |
Or change the return type to JSValueConst but that's much less idiomatic (even internally we only have three functions that do that.)
| } | ||
| #endif | ||
| m->resolved = true; | ||
| ctx->import_assertion = m->import_assertion; |
There was a problem hiding this comment.
| ctx->import_assertion = m->import_assertion; | |
| ctx->import_assertion = js_dup(m->import_assertion); |
| if (js_resolve_module(ctx, m1) < 0) | ||
| return -1; | ||
| } | ||
| ctx->import_assertion = JS_UNDEFINED; |
There was a problem hiding this comment.
| ctx->import_assertion = JS_UNDEFINED; | |
| JS_FreeValue(ctx, ctx->import_assertion); | |
| ctx->import_assertion = JS_UNDEFINED; |
| JSValueConst *resolving_funcs = argv; | ||
| JSValueConst basename_val = argv[2]; | ||
| JSValueConst specifier = argv[3]; | ||
| ctx->import_assertion = argv[4]; |
There was a problem hiding this comment.
This looks like it should either js_dup or the JS_FreeValue further down shouldn't be there. argv is JSValueConst, meaning the caller doesn't transfer ownership.
|
It's late for me too, will do tomorrow, thanks for the review so far, will take better look at it tomorrow as well |
|
Took me a bit longer than expected as things came up, but managed to fix memory leaks for imports and JSON modules, still needs a bit more further testing to ensure no memory leaks and figure out something with dynamic imports as they still leak as for test262, I forgot to enable tests but I remember looking at the repository, it is missing import assertion tests for some reason, well, I can still enable them |
|
Not sure I understand the purpose of Shouldn't we change the signature of the module loader and pass in an extra "assertions" argument? |
|
First time I worked on this patch (some time ago), this is how it worked, but I wanted to avoid creating breaking changes. If this is fine, I can implement it this way |
I'd say it's OK to break it here. @bnoordhuis thoughts? |
|
Closing as it's already been implemented in QuickJS and no longer needed, so it doesn't take space. |
3 participants
Rather trivial implementation of handling import assertions and JSON modules, I have explained some of the choices in the comments, hopefully can get things fixed and tests passing, setting the PR as draft for now until it's ready. Fixes #780.
If anyone has any suggestions, I'll be glad to implement them.
Thanks in advance.