WIP: Implement Import assertions and JSON modules #1039
Conversation
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| @@ -472,6 +472,10 @@ struct JSContext { | |||
|
|
|||
| struct list_head loaded_modules; /* list of JSModuleDef.link */ | |||
|
|
|||
| /* 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.
| @@ -855,6 +859,9 @@ struct JSModuleDef { | |||
| bool eval_has_exception; | |||
| JSValue eval_exception; | |||
| 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.
quickjs.c
Outdated
| @@ -28479,6 +28498,11 @@ JSValue JS_GetModuleNamespace(JSContext *ctx, JSModuleDef *m) | |||
| return js_dup(m->module_ns); | |||
| } | |||
|
|
|||
| 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.)
quickjs.c
Outdated
| @@ -28504,6 +28528,7 @@ static int js_resolve_module(JSContext *ctx, JSModuleDef *m) | |||
| } | |||
| #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); |
quickjs.c
Outdated
| @@ -28517,6 +28542,7 @@ static int js_resolve_module(JSContext *ctx, JSModuleDef *m) | |||
| 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; |
quickjs.c
Outdated
| @@ -29042,6 +29068,7 @@ static JSValue js_dynamic_import_job(JSContext *ctx, | |||
| 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? |
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.