Add a ContextData struct to inject host defined types from the context

[hansl]

No description provided.

[codecov]

Codecov Report

Attention: Patch coverage is 52.17391% with 11 lines in your changes are missing coverage. Please review.

Project coverage is 50.27%. Comparing base (6ddc2b4) to head (911052e).
Report is 139 commits behind head on main.

❗ Current head 911052e differs from pull request most recent head 757554a. Consider uploading reports for the commit 757554a to get more accurate results

Files	Patch %	Lines
core/interop/src/lib.rs	53.33%	7 Missing ⚠️
core/engine/src/context/mod.rs	50.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3802      +/-   ##
==========================================
+ Coverage   47.24%   50.27%   +3.03%     
==========================================
  Files         476      459      -17     
  Lines       46892    44968    -1924     
==========================================
+ Hits        22154    22608     +454     
+ Misses      24738    22360    -2378     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jedel1043]

I'm a lot more on the fence about this change.

I think the behaviour of the new traits is very obvious at first glance: convert the original JsValue arguments into the provided types, in positional order. However, adding this API would add an arbitrary conversion that doesn't really map as cleanly to the current semantics, since there are a LOT of places from where a HostDefined could come from: the current Script, the current Module, the current Realm or (in the near future) the current Context; all of them accessible from the passed context.

That being said, what problem does this new API try to solve? I think we could find an alternative that has more "predictable" semantics than the current proposal.

[hansl]

I have state that I need to pass down to the JS API. This allows me to pass the state around without having to mess with the context, which has a surprising amount of boilerplate.

It also reduces API friction and semantic mapping; you don't need to worry too much about Context or Realm or other objects from Boa. You can focus on your own code.

This is what my code looks like: https://github.com/hansl/golem/blob/golem-script/src/golem-script-host/src/modules/golem/db.rs. Check the query_one_ function, for example. I also just saw that I can now clean up the return values and save some more boilerplate.

I'm willing to listen to alternatives, but it's common for dependency injection to not worry too much about order of arguments. People will order them as they see fit; whether it's app-specific arguments first, then positional arguments, then environmental context, or whatever order. I'm not sure I see the confusion. I really think this improves coding in complex applications.

See Rocket/Axum for good examples of dependency injection where the order of arguments is very loose.

[jedel1043]

I'm willing to listen to alternatives, but it's common for dependency injection to not worry too much about order of arguments. People will order them as they see fit; whether it's app-specific arguments first, then positional arguments, then environmental context, or whatever order. I'm not sure I see the confusion. I really think this improves coding in complex applications.

I'm not really worried about the order of arguments per se. What I'm really worried about is the HostDefined type itself, which is so opaque about what it does that a first-time user could be confused on where exactly the argument comes from.

I'm aware that the docs could explain where to set the values for HostDefined, but I'm also aware that not everyone is as thorough as to read all the documentation to see what exactly this new shiny type wrapper does. Some people will just define a function with a HostDefined argument, then try to set it on many of the wrong places that HostDefined exists (as mentioned, the host defined map is on a lot of places throughout the engine, and will definitely be on the context itself in the future) and be confused on why the function is missing a host defined argument that they definitely set on their code.

I have state that I need to pass down to the JS API. This allows me to pass the state around without having to mess with the context, which has a surprising amount of boilerplate.

Maybe this can be solved in a clearer way by having the aforementioned HostDefined on the context itself, with some utility functions that avoid error handling boilerplate.
Having:

let f = (|HostDefined(host): HostDefined<CustomHostDefinedStruct>| {
    host.counter + 1
    }).into_js_function_copied(&mut context);

is barely equal in length to:

let f = (|context: Context| {
    let counter = context.host_defined_mut().try_get::<CustomHostDefinedStruct>()?;
    *counter += 1
    }).into_js_function_copied(&mut context);

But on the second version you can tell at first glance from where the host defined data is fetched, and it also avoids cloning the data!

EDIT: We could even make the host defined data first-class, and have special functions to just pull the data directly from the context:

let f = (|context: Context| {
    let counter = context.try_get::<CustomHostDefinedStruct>()?;
    *counter += 1
    }).into_js_function_copied(&mut context);

[hansl]

There is another advantage to my approach here of using injection like those to pass data; the error handling of transformation/reaching for that data is moved outside of the function itself.

Ironically, only your first example would compile (the one that uses the HostDefined argument), because the compiler wouldn't be able to infer the full return type of your other examples (remember that ? calls Into::into). For example 3 to compile, you'd need to add the following:

let f = (|context: Context| -> JsResult<()> {
    let counter = context.try_get::<CustomHostDefinedStruct>()?;
    *counter += 1;
    Ok(())
}).into_js_function_copied(&mut context);

My code is significantly simpler at this point. And the user doesn't have to worry about return types.

let f = (|HostDefined(counter): HostDefined<Counter>| counter + 1).into_js_function_copied(&mut context);

The biggest advantage of yours is the mutability reference of the original, instead of cloning. I couldn't get the reference to work for this since the Context lifetime is weird. I can try again tomorrow.

[hansl]

What I'm really worried about is the HostDefined type itself, which is so opaque about what it does that a first-time user could be confused on where exactly the argument comes from.

Naming things is hard. InjectHostDefined maybe? I'm trying to stay short so it doesn't turn into Java. Maybe just Inject and discuss in comments and examples what is there to inject.

[jedel1043]

Naming things is hard. InjectHostDefined maybe? I'm trying to stay short so it doesn't turn into Java. Maybe just Inject and discuss in comments and examples what is there to inject.

I'm also not really worried about the name itself. I'm mostly worried about the concept of having a host defined argument that isn't really clear where it comes from without reading the implementation of TryFromJsArgument for HostDefined.

[jedel1043]

My code is significantly simpler at this point. And the user doesn't have to worry about return types.

That's simply because I decided to show the mutability of the proposed API. If we want to compare it with your API in a fair way, it would be something like:

let f = (|context: &mut Context| {
    context.try_get::<CustomHostDefinedStruct>().map(|h| h.counter + 1)
}).into_js_function_copied(&mut context);

Which is still a bit longer, but without hidden assumptions about where the data comes from.

[hansl]

Following offline discussion, there can be multiple HostDefined containers (depending on the Realm), and currently there's no Context-specific HostDefined, which caused the confusion (I always played with a single realm).

Changed the PR to add one, and changed this new injector to reflect that the Data comes from Context. We might extend to other semantics in other injectors later (e.g. having multiple realm merged into one, etc).

[jedel1043]

Pretty satisfied with this new API. Thank you for the good work!

[HalidOdat]

Looks great! Just needs a rebase :)

[hansl]

@HalidOdat Done. PTAL.