Dynamic import

[mookums]

This adds support for dynamic imports of modules (eg. await import("xyz")). This is in draft at the moment as it is dependent on lightpanda-io/zig-v8-fork#83 as it requires the isPromise().

[karlseguin]

Is this necessary? I would have thought that you can use resource_str directly as the key to context.module_identifier.

[mookums]

Sometimes using the resource works but other times like below, it doesn't.

INFO  js : dynamic import . . . . . . . . . . . . . . . . . . [+0ms]
      specifier = ./shreddit-overflow-menu-31f50e45.js
      resource = ./comment-client-js-4adc8893.js
      referrer_full = https://www.redditstatic.com/shreddit/en-US/comment-client-js-4adc8893.js
      normalized_specifier = ./shreddit-overflow-menu-31f50e45.js

[karlseguin]

We should look into this. I started to look into this, but the crash on 1620 (and, to a lesser extent, the logging of opaque values), kind of stopped me.

I think the resource value is always supplied by us, and, for correct caching, it should always be the full url. We should find out where we're submitting the non-full script name ./comment-client-js-4adc8893.js and fix it (maybe there's a combination of of base + url that stitch doesn't handle properly).

(It's possible the name is somehow derived by v8, and we have no choice, but that seems unlikely, and I rather make sure that, because it smells like a bug to me..and fixing it wouldn't just fix potentially weird caching issue, it would let you remove this block of code).

[karlseguin]

After fixing the crash, when I looked at this for reddit, the resource was always a full url, including for comment-client-js-cc940b94.js

[mookums]

I never really crashed running this but I switched from call_arena tocontext_arena and now the resource_str works so I assume that's also how you fixed the crash?

[karlseguin]

I think it's worth exploring whether dynamicModuleCallback can leverage the existing module function. module doesn't have its own try catch, so I think the rejection parts shouldn't be an issue:

var try_catch: TryCatch = undefined;
try_catch.init(context);
defer try_catch.deinit();

// change module to return the module so that you can evulate it, I gues
const m = self.module(....) catch |err| {
  var error_message: ?[]const u8 = null;
  if (try_catch.hasCaught()) {
      // ...
  } else switch (err) {
     // if you want to handle specific errors from module(...)
  }
}

Not having to duplicate the specifier normalization, cache management and module compilation/evaluation seems like a win.

[mookums]

Ended up changing module() so it now returns !?v8.Promise where it returns null when the module is already in the cache and returns the evaluation result, which should always be a v8.Promise.

I ended up removing moduleNoCache() and adding the functionality to the normal module() because it felt like a lot was being duplicated across the two.

[karlseguin]

I don't see how it's possible, but when I tried this branch with reddit, context.module_cache.get(specifier) returns null (and this code crashed). But I don't see how context.module() called with cacheable: true can return without inserting that entry...

[karlseguin]

The call_arena isn't safe to use, e.g.

 const specifier_str = jsStringToZig(context.call_arena, specifier, iso) catch {

The exact lifetime of the call_arena is a bit unspecific, but it's guaranteed to exist for at least the point where a WebAPI function is called (e.g. Window._setTimeout) to the point where data is handed to v8.

Loading a module is a lot wider than that and likely includes many WebApi calls. So imagine you have a module:

// awesome.js
console.log("hello");
// <-- should assume that the call_arena is reset at this point

Your specifier (and probably other variables) are created before this code executes, but those allocations will become invaild as soon as console.log returns.

[karlseguin]

We should look into this. I started to look into this, but the crash on 1620 (and, to a lesser extent, the logging of opaque values), kind of stopped me.

I think the resource value is always supplied by us, and, for correct caching, it should always be the full url. We should find out where we're submitting the non-full script name ./comment-client-js-4adc8893.js and fix it (maybe there's a combination of of base + url that stitch doesn't handle properly).

(It's possible the name is somehow derived by v8, and we have no choice, but that seems unlikely, and I rather make sure that, because it smells like a bug to me..and fixing it wouldn't just fix potentially weird caching issue, it would let you remove this block of code).