dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.82k stars 4.61k forks source link

Improve the JavaScript interop code generators to support additional scenarios #78455

Open yugabe opened 1 year ago

yugabe commented 1 year ago

Is there an existing issue for this?

Is your feature request related to a problem? Please describe the problem.

I'm trying to invoke a .NET method via JS interop that passes an array of JavaScript objects back.

[JSImport("myFunction", "myModule")]
public static partial void MyFunction(Action<JSObject[]> callback);

The above declaration gives a compilation error from the generator specifically for the callback symbol: SYSLIB1072: The type 'System.Action<System.Runtime.InteropServices.JavaScript.JSObject[]>' is not supported by source-generated JavaScript interop. The generated source will not handle marshalling of parameter 'callback'.

I tried variations of [JSMarshalAs<T>] attributes on the parameter, but the error seems to indicate this type is not supported by design (other parameters, such as Action<JSObject> ask for the parameter to be annotated).

The related documentation page seems to imply a callback that supplies a JavaScript object array should be supported (the below images are edited for clarity):

image

image

The object on the JavaScript side is not straightforward or viable to manually serialize (it contains DOM objects and circular references). I can transform the object on the JavaScript side, but doing so would still result in an array of objects, which are not marshalled as per the issue described above.

Describe the solution you'd like

Seeing as how arrays of objects are already supported (as parameters and return values), this is possibly just a missed use case which should be supported. Currently the workaround is to manually call back from JavaScript to .NET without passing the callback directly to the generated interop method, which is insufficient for code organization.

The problem in itself obviously exists because there is no straightforward way to interact with the DOM without custom user code. An alternative (arguably better) solution would be if there was a way to directly interact with the DOM from browser-wasm .NET apps (like Blazor). The Blazor-included JavaScript interop supports special ElementReference objects that can hold reference to DOM objects either instantiated in Blazor or outside of it, which makes this problem a bit easier to solve with the (slower) alternative interop solution. As the current implementation can use proxy objects, it might be worth considering using proxy objects that support more scenarios (like calling methods on them directly), and JavaScript event handling.

Additional context

  1. I haven't tried if marshalling as JSType.Function<JSType.Any> would work if I used Action<object> as a parameter.

  2. It's also a bit confusing to annotate Action and Func types with the JSType.Function parameters, because I'm not sure if the type parameters should always match (especially when a function returns JavaScript void - JSType.Void), but this is most probably a documentation issue instead.

  3. It would also be worth clarifying why some parameters need to be explicitly annotated, like Action.

dotnet-issue-labeler[bot] commented 1 year ago

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

yugabe commented 1 year ago

I hope @pavelsavara won't mind me pinging.

ghost commented 1 year ago

Tagging subscribers to 'arch-wasm': @lewing See info in area-owners.md if you want to be subscribed.

Issue Details
### Is there an existing issue for this? - [X] I have searched the existing issues ### Is your feature request related to a problem? Please describe the problem. I'm trying to invoke a .NET method via JS interop that passes an array of JavaScript objects back. ```csharp [JSImport("myFunction", "myModule")] public static partial void MyFunction(Action callback); ``` The above declaration gives a compilation error from the generator specifically for the `callback` symbol: `SYSLIB1072: The type 'System.Action' is not supported by source-generated JavaScript interop. The generated source will not handle marshalling of parameter 'callback'.` I tried variations of `[JSMarshalAs]` attributes on the parameter, but the error seems to indicate this type is not supported by design (other parameters, such as `Action` ask for the parameter to be annotated). The related [documentation page](https://learn.microsoft.com/en-us/aspnet/core/blazor/javascript-interoperability/import-export-interop?view=aspnetcore-7.0) seems to imply a callback that supplies a JavaScript object array should be supported (the below images are edited for clarity): > ![image](https://user-images.githubusercontent.com/14803737/202159720-975cd168-74a9-4bcb-bb46-00dbf51a4f59.png) > > ![image](https://user-images.githubusercontent.com/14803737/202158777-30d3a54e-aab3-46c9-8c12-a12e52b39ee7.png) The object on the JavaScript side is not straightforward or viable to manually serialize (it contains DOM objects and circular references). I can transform the object on the JavaScript side, but doing so would *still result in an array of objects*, which are not marshalled as per the issue described above. ### Describe the solution you'd like Seeing as how arrays of objects are already supported (as parameters and return values), this is possibly just a missed use case which should be supported. Currently the workaround is to manually call back from JavaScript to .NET without passing the callback directly to the generated interop method, which is insufficient for code organization. The problem in itself obviously exists because there is no straightforward way to interact with the DOM without custom user code. An alternative (arguably better) solution would be if there was a way to directly interact with the DOM from `browser-wasm` .NET apps (like Blazor). The Blazor-included JavaScript interop supports special `ElementReference` objects that can hold reference to DOM objects either instantiated in Blazor or outside of it, which makes this problem a bit easier to solve with the (slower) alternative interop solution. As the current implementation can use proxy objects, it might be worth considering using proxy objects that support more scenarios (like calling methods on them directly), and JavaScript event handling. ### Additional context 1. I haven't tried if marshalling as `JSType.Function` would work if I used `Action` as a parameter. 2. It's also a bit confusing to annotate `Action` and `Func` types with the `JSType.Function` parameters, because I'm not sure if the type parameters should always match (especially when a function returns JavaScript `void` - `JSType.Void`), but this is most probably a documentation issue instead. 3. It would also be worth clarifying why some parameters need to be explicitly annotated, like `Action`.
Author: yugabe
Assignees: -
Labels: `arch-wasm`, `untriaged`, `area-System.Runtime.InteropServices.JavaScript`
Milestone: Future
pavelsavara commented 1 year ago

Thanks for the feature request.

Answering the questions: 1) That should work I think. Also JSObject proxy could be used to proxy for JS array. That is if you don't need to access the items on C# side. 2) on JS side, there is no difference between void and non-void function. 3) In the complex scenarios the developer intention is expressed and captured for better future compatibility. Since this is new API, we may want to change the default marshaling and that would be breaking change. So we rather enforce explicit expectation. We may be able to relax some if it in the future, but not the other way around.

yugabe commented 1 year ago

Thanks for the response and the answers! I see very much potential in the new API, seeing as how I personally saw orders of magnitudes of performance wins in some cases. Getting to and back from the DOM is cumbersome, I hope it will become easier with future improvements.

Some reactions to the answers:

  1. Yeah, I could use the JSObject, but I could only iterate the properties; not the array indexes, as you say. Although, it just occurred to me that might work to use the indexes as a string, seeing as how ["a", "b", "c"]["1"] actually does return b as the item at index 1. It feels hacky though.

  2. The problem isn't really on the JS side in this case. It's how the return value is not explicitly described. An Action<string> and a Func<string> both map to JSType.Function<JSType.String>? It would be clearer and more explicit if an Action was JSType.Function<JSType.Void>, so essentially the first parameter if the Function was always TResult. Now it's confusing if both map to the same, when in reality one takes a parameter, and the other returns a value and takes none.

  3. I was feeling something like this was the reason and I accept it. It was unclear from the documentation why this was needed (it states JSMarshalAs is optional). Could you give an example of what an alternative way to marshal an Action would be? With Func and Action with type arguments, I understand needing to be explicit, but seeing as how string's marshaling is implied, maybe Action could be too (if there really is no other way to marshal a callback than as a function proxy).

pavelsavara commented 1 year ago
the first parameter if the `Function` was always `TResult`. 

It maps 1:1 to generic parameters on C# side.

Could you give an example of what an alternative way to marshal an `Action` would be? 

I don't know yet. Maybe different GC semantics, maybe different type produced on JS side, maybe different implementation of the wrappers.

implied, maybe Action could be too

Technically yes, we have all the type information from the C# signature, if there is no alternative marshaling for it.

hoangdovan commented 1 year ago

Hello @yugabe, Do you have any work around for this problem? I have similar situation and got trouble with this new API. MS deprecated InvokeUnmarshalled() but new API still cannot replaced for the old method in some case. And do you have any guide how to working with JSObject? I searched for this but cannot see any useful info! Thanks!

yugabe commented 1 year ago

Hi @hoangdovan, I haven't taken a deeper look at it, but generating the API by using JSType.Any and then casting it to the appropriate array type should work, but I haven't tested it myself.

You can always do it by not directly passing a callback, but actually calling a different .NET-published JSExported method from the client side. It's not too elegant, but it should work.

JSObject, as far as I can tell, isn't really anything special. It's a proxy object you can use to get the properties of the object in JavaScript (you can even use GetPropertyAs*("0") to get the first element in an array, for example). I don't think it's well documented, but I assume the method calls are interop boundaries, so communication happens when you call the method. You can pass these objects to, or recieve them as return values from methods annotated with JSImport/JSExport. I think you should open an issue if you have a specific problem, even if it's with the documentation. You can go to the bottom of the docs page in question and it'll help you in opening an issue in the relevant repo.

hoangdovan commented 1 year ago

Hi @yugabe , maybe I will try to request for add new feature to support more for this API. Thank you for your information!

SeanCapes commented 1 year ago

Following on from this, what's the recommended approach to pass and use arrays in C# from JavaScript? As mentioned above, huge overhead on marshalling performance using JSON. We're also considering MemoryView, but have just started looking at that. Ideal would be the ability to use the JSObject directly. We're trying to pass and use a multi-dimensional to then create a Dictionary equivalent. Any advice greatly appreciated. Thanks

pavelsavara commented 1 year ago

Following on from this, what's the recommended approach to pass and use arrays in C# from JavaScript?

It really depends on arrays of what. There is table with supported combinations here https://learn.microsoft.com/aspnet/core/client-side/dotnet-interop

As mentioned above, huge overhead on marshalling performance using JSON.

There is request to add convinience wrapper for that, but it would have roughly the same perf as what you could do already with string marshaling. https://github.com/dotnet/runtime/issues/77784

Ideal would be the ability to use the JSObject directly.

There is another proposal for that https://github.com/dotnet/runtime/issues/78905 Note that there would be interop call (not very cheap) for each get or set operation when this is implemented.

Some of it is already possible with the current JSObject API

We're trying to pass and use a multi-dimensional to then create a Dictionary equivalent. Any advice greatly appreciated. Thanks

Passing array of keys and array of values and then recombining them on the other side worked well for me.

SeanCapes commented 1 year ago

Thanks @pavelsavara, we'll try that. Assume overhead of marshalling arrays of strings due to lack of support in WebAssembly?

pavelsavara commented 1 year ago

Thanks @pavelsavara, we'll try that. Assume overhead of marshalling arrays of strings due to lack of support in WebAssembly?

My comment about perf is to differentiate costs of interop calls vs usual C# to C# calls expectations. Unless you do very large colections or run interop calls in a tight loop, the perf is usually OK.

Even if WASM had native string, we would need .NET string data structure in wasm memory. So, we are marshaling underlying UCS2 bytes when we marshal (non-interned) strings. Allocation and char-by-char copy. The array is copy (by value) in this case. So allocation and subsequent GC.

(In context of this issue, I speak about JSImport interop, not default Blazor interop which could also have network transport costs on top of it in hybrid mode)

SeanCapes commented 1 year ago

@pavelsavara Thanks for your support so far. The other thing we're considering is a MemoryView, are there any available examples of usage? I appreciate that this is work in progress. The primary NFR for us is speed. Once again, thanks for any advice offered.

pavelsavara commented 1 year ago

ArraySegment<byte> marshalled as MemoryView to JS side and kept as shared buffer for long time is the most performant alternative. Marhaling ArraySegment on each call of tight loop is not good idea because it has the overhead of GC on both sides.

For ArraySegment<byte> example we had it in the WebSocket implementation temporarily, but I optimized it further exactly because of the GC pressure it could create under heavy load. https://github.com/dotnet/runtime/commit/2a2edc89880ba469e561a7b8ddbfb4f422ddd6a1

In that case Span<byte> could be better but has lifetime only for the duration of the call to JS (when it's on current stack), so it's not async friendly.

For examples, please see GetResponseBytes.

It's called from async method here but the call itself is synchronous.

And the implementation of http_wasm_get_response_bytes is here.

And finally you could pin the memory yourself and marshal IntPtr. In that case you need to know even more about what you are doing.

It's complex code, so I'm sure this is not great tutorials. Sorry.

SeanCapes commented 1 year ago

Hey, thanks for any advice and help offered, all appreciated. Understand with nascent pieces this happens, its part of new tech.

SerratedSharp commented 10 months ago

Maybe there should be a "partially" supported indicator for Arrays of JSObjects in the documentation's supported types matrix with some footnotes on it not being supported in promise returns or action callback parameters? And/or clarify that it is only supported via representation as a single JSObject for the array, and thus the function signatures/marshaling cannot specify an array in that case.

SerratedSharp commented 10 months ago

My workaround for an action wanting to receive an array of JSObject's. Unfortunately requires an extra interop call to unpack array references


        [JSImport(baseJSNamespace + ".BindListener")]
        public static partial JSObject BindListener(JSObject jqObject, string events,
            // action where second function param will be an ArrayObject reference
            [JSMarshalAs<JSType.Function<JSType.String, JSType.Object>>] Action<string, JSObject> handler);

        // Used for unpacking an ArrayObject into a JSObject[] array
        [JSImport(baseJSNamespace + ".GetArrayObjectItems")]
        [return: JSMarshalAs<JSType.Array<JSType.Object>>]
        public static partial JSObject[] GetArrayObjectItems(JSObject jqObject);

Javascript (I probably didn't need ArrayObject class, but makes the pattern clearer)

    JQueryProxy.BindListener = function (jsObject, events, action)
    {
        let handler = function (e) {
            var someArray = [jQuery('div'),jQuery('#x')];
            // pack array into single reference, and call action passing the array
            action("blah", new ArrayObject(someArray));
       }.bind(jsObject);

        jsObject.on(events, handler);
        return handler; // return reference to the handler so it can be passed later for .off
    }

class ArrayObject {
    constructor(items) {
        this.items = items;
    }
}

The action listener that will receive the ArrayObject and unpack it into an actual array

// action that wants to receive an array as a param
Action<string, JSObject> interopListener = 
    (eventEncoded, arrayObject) => {       
        // unpack the single ArrayObject JSObject reference into its individual elements
        JSObject[] unpackedArray = HelpersProxy.GetArrayObjectItems(arrayObject);    
        // ...        
    };

JQueryProxy.BindListener(this.jsObject, events, interopListener);
maxkatz6 commented 4 months ago

Supporting JSType.Function with more than 4 parameters is also needed for callbacks. Without that we have to pass a JSObject as a single param and call nested JS interop to fetch each property independently. Which will eventually be unsupported in .NET 9 WT as per https://github.com/dotnet/runtime/issues/85592#issuecomment-2031876112.