Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Robust js interop #70

Merged
merged 33 commits into from
May 22, 2020
Merged

Conversation

Joelius300
Copy link
Contributor

@Joelius300 Joelius300 commented Jan 4, 2020

This PR improves the interop-layer to work with delegates instead of bare static- and instance-methods which allows for much more flexibility (but maybe worse performance. I don't expect it but tests may be required).

On the javascript-side, the interop is much more flexible and reusable because it uses ...args which allows for any number of arguments. If we didn't have this approach we'd need tests and special cases for all different callback-types which would be horrible.
On the c#-side, it's gotten more restrictive in a good way (following the type-safety of c#). You now have a fixed c# delegate which declares the signature of the callback. This allows you to only pass in correct delegates and if you use a javascript-handler you can immediately see what arguments you can expect. In many cases, we just use JsonElement since it's hard to model the actual type that's returned or it just hasn't been done yet. But wherever possible we use System.Text.Json nope, we should use the same thing for both serializing and deserializing which currently is Json.Net to deserialize to the actual type, meaning that we check the delegate parameters and if it's not object or JsonElement we deserialize it in order to invoke the delegate with actual types.

This PR is a draft because we could actually use Json.Net for the deserialization part which would make the converters much easier probably and I did not realize that before (see b4883b7 and it's comment). Therefore I'll probably play around with that some more. Also ObjectEnum can't be read currently, so that's also something that needs to be done before merging.

Another point which I don't like about the PR currently is the sample for the legend callbacks. While it demostrates how to use it, you can see that it has really bad performance. Unlike with other c# callbacks (see the other samples), this one somehow has big performance issues (maybe because of the many invokes but that seems unlikely given how many times the OnHover callback is executed without lag).

One more issue is support for value callbacks on server-side. The dotnet dispatcher from javascript only allows async calls to c# on the server-side variant which means that it will return a promise and that is of course not the value we want to give to chart.js. The client-side dispatcher allows sync calls so we can pass the actual value to chart.js and it works. One thing we could do for this is ask the asp .net team if they will ever allow sync calls to c# in server-side blazor. A SO question has already been asked but more or less only confirmed my worries.

When we split the projects (#78), we will probably keep IMethodHandler in ChartJs.CSharp but move the DelegateHandler and the JavaScriptHandler to ChartJs.Blazor in order to minimize the required dependencies.

This PR has a few side-effects like renaming xAxes to XAxes in c# and adding/removing a few properties which were not yet present or wrong.

I don't know if adding more callbacks should be part of this PR, maybe someone has an opinion on this. Adding them in this PR would give more room for tests but I think it might be better to keep this PR at a minimum and make it extensible(!!!).

Joelius300 added a commit to Joelius300/ChartJs.Blazor that referenced this pull request Mar 26, 2020
This adds the animation configuration to all options. It now also supports easing options. This closes mariusmuntean#74 because it makes it obsolete. The callbacks can only be done after mariusmuntean#70 but they're added as todo comments.
@Joelius300

This comment has been minimized.

This class had mostly helper methods for the now replaced click and hover handlers. The null or whitespace checks are inlined everywhere so this class isn't used anymore. This was also mentioned in this [PR-comment](mariusmuntean#94 (comment)). If we have a helper class for argument validation, we need to actually use it.
@Joelius300 Joelius300 mentioned this pull request Mar 30, 2020
@Joelius300

This comment has been minimized.

@Joelius300

This comment has been minimized.

This commit removed cycle.js and uses a replacer method along with JSON.stringify to serialize the object already before actually passing it to C#. This is a performance improvement because blazor doesn't have to deserialize the object, only that string. We want a string anyway since we deserialize using json.net (custom converters, consistency, etc) so we can get rid of that conversion. If you don't have a type to deserialize the callback parameters to, you now use json.net objects like JObject, JArray and JValue instead of JsonElement.
When the callback receives a large object, like chart-data in the legend item filter (around 12000 chars stringified), it takes a rather long time deserialize it. Applying this new attribute will skip deserialization for that parameter and the default value will be passed.
Additionally to the last commit where deserialization (C# side) was disabled for parameters with a IgnoreCallbackValueAttribute, this commit disabled the process of stringifying the parameter (JS side) which further improves performance.
@Joelius300 Joelius300 marked this pull request as ready for review April 9, 2020 13:51
@Joelius300 Joelius300 requested a review from mariusmuntean April 9, 2020 13:51
@Joelius300
Copy link
Contributor Author

From my side this PR is done. It won't be merged into master (!) but it will be the first big change for the upcoming 2.0 release and will be integrated there. There's a good change the merging has to be done by hand in order to preserve a good commit history. I'll do that once the changes are approved and the version stuff including milestones is done.

@mariusmuntean Could you please look over it? All the samples have been adjusted accordingly and the performance has been improved by a lot. You can test it by cloning my repo.

@Joelius300 Joelius300 added this to the 2.0.0 milestone Apr 9, 2020
@Joelius300 Joelius300 mentioned this pull request Apr 12, 2020
@Joelius300 Joelius300 mentioned this pull request May 17, 2020
A lot of general improvements including correct casing, better readability and potential bug fixes. Still not a js/ts expert but it should be an improvement overall.
Chart.js records what changes where made to the data and animates the chart accordingly. To get the same behaviour we would also have to record, emit and apply such metadata which can be considered as a later feature. For now, we just replace the reference so Chart.js doesn't think all the data is fresh.
@Joelius300 Joelius300 merged commit 5218e71 into mariusmuntean:master May 22, 2020
@Joelius300 Joelius300 deleted the robust-js-interop branch May 22, 2020 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants