mariusmuntean / ChartJs.Blazor

Brings Chart.js charts to Blazor
https://www.iheartblazor.com/
MIT License
677 stars 151 forks source link

Callbacks with return value not supported on server-side Blazor #90

Open Joelius300 opened 4 years ago

Joelius300 commented 4 years ago

With #70 I have reworked the interop-layer once more to provide a type-safe way of invoking any delegate from JavaScript without writing much new code on either side for different callbacks. Apart from that I also implemented the first case of callbacks that return a value (namely Custom Ticks Format callback). The procedure of such a callback is as follows:
Chart.js will first call from JavaScript into C# with some arguments which will be deserialized (that's why we need #84 and more). Then on the C# side, we can handle these arguments and produce a response that will be serialized again and returned to the JavaScript caller.

The chart.js callbacks are synchronous, not async (at least I haven't seen any). That means they expect the actual value to be returned, not a promise (or generator). This obviously means that the value has to be returned to the chart.js handler synchronously. On client-side this is quite easy because the JavaScript dispatcher from wasm blazor has the ability to invoke C# from JavaScript synchronously. But now lets talk about server-side. Server-side blazor is built on SignalR which is built on websockets. JavaScript websockets and SignalR are async (I don't know either very well but afaik they only work async). That means there is just no way to do synchronous JavaScript-C# interop.

What have I tried/considered without success:

Now the question is what to do. In the current (#70) implementation of the interop layer, it just writes a warning in the browser with console.warn() and falls back to the default handler if you supply a C# handler to a callback that expects a return value. JavaScript handlers are still fully supported and work perfectly fine even with return values.
Now I would like to know some opinions on this. What should happen when it's not supported? Is there still hope to make this work? Because I genuinely don't see any way to make this work for server-side. Is this warning enough? Where do we document this that it's discoverable and no one wastes time trying to get it to work?

What do you think?

Additional stuff

cc @mariusmuntean, @netcorefan

SeppPenner commented 4 years ago

If you ask me, I would leave the warning as it is and document this in the wiki. You have already tried a lot of things and there doesn't seem to be a solution (yet). I',m not an expert for JavaScript, but this seems to be quite complicated.

MindSwipe commented 4 years ago

I agree with SeppPenner to leave the warning and fall back to the default handlers, but in the mean time it might be worth opening an issue with the Chart.js folks asking if it is possible (they might know more about it) and if it is not what to do instead

Joelius300 commented 4 years ago

I really doubt that there is a way but I'll try to find out more (not on their repo, they don't like that). For handling this case at runtime, I think sticking with the browser warning seems to be the best way.

Edit: SO-Question here.

thomasclaudiushuber commented 4 years ago

Hey @Joelius300,

it's a great question.

I don't have a proper solution, but I can say that you're not alone with this problem..

In one application I did a port of Ag-Grid to Blazor, and I had exactly the same issue. Everything works good, selection, column definitions etc. But the Ag-Grid allows also custom cell renderers. And these are synchronous JavaScript functions that return a piece of HTML.

I got it working without issues in Blazor WASM with synchronous JS calls. But in Blazor Server, I was tackling exactly the same problems like you.

Cheers, Thomas

Joelius300 commented 4 years ago

@thomasclaudiushuber That's unfortunate to hear. I'm glad I'm not missing something obvious but I don't have anymore ideas to get this to work. It just seems impossible :(

Joelius300 commented 4 years ago

From my side #70 is now complete. I've decided to keep the warning and document it accordingly once we're done. It'll be in the wiki and the readme as well. This change is part of release 2.0 where we'll also need to invest some time into better documentation.

It's unfortunate that this can't be supported on server-side but as of today, there appears to be no way to make it possible.

I will remove the discussion label to this. However, it'll stay open for any progress made in this area. I highly doubt that this is something we can support but it's definitely something we want to support so it'll stay. Because this will be a big question asked by many, this issue will be pinned again once the feature releases.

We're very grateful for any insights you can share on this.