search

Joelius300 / ChartJSBlazor

This library is a modification of the awesome ChartJs.Blazor library by mariusmuntean. It's supposed to add more functionality to the LineChart and generally make the library more complete.
Other
40 stars 6 forks source link

Migration to .NET Core 3.0 Preview 9 #73

Closed cbovar closed 5 years ago

cbovar commented 5 years ago

Upgraded to .NET Core 3.0 Preview 9

Joelius300 commented 5 years ago

The update to pre9 seems good but why did you decide to use ValueTask? As far as I'm concerned that isn't required for the update and it should only be used if an async method will complete synchronously most of the time (see here). This method will never complete synchronously so using ValueTask is almost certainly incorrect.

By the way the methods you changed have an issue (which existed before your commit as well, I had it in my backlog). If you directly return the task instead of awaiting it inside a try-catch block, the exception will never be caught. Thats something we need to fix but it has nothing to do with preview9 so I believe it doesn't belong in this PR.

Joelius300 commented 5 years ago

Never mind the ValueTask Stuff, I hadn't read the notes yet (I'm actually in class currently lol).

SeppPenner commented 5 years ago

@Joelius300: I don't know how you want to handle this. As far as I can see, my pull request (https://github.com/Joelius300/ChartJSBlazor/pull/75) additionally updates the readme, removes some unused usings and fixes the onclick handlers as well as the routing in App.razor. The rest is the same as this one.

My pull request is tested and I assume this one not because otherwise @cbovar would have seen that the onclick handlers don't work like this properly anymore. (You are getting runtime errors in the browser like this

Error: There was an error applying batch 2.
DOMException: Failed to execute 'setAttribute' on 'Element': '@onclick' is not a valid attribute

and the pages are rendered but the menu disappears). This is already described in https://devblogs.microsoft.com/aspnet/asp-net-core-and-blazor-updates-in-net-core-3-0-preview-9/ under Troubleshooting guide.

I vote for merging my pull request and closing this one because mine is more complete, but it's up to you @Joelius300.

cbovar commented 5 years ago

I haven't experienced those issues on my side while testing it. Anyway, the other PR looks more complete, I'll close that one.

SeppPenner commented 5 years ago

Sorry by the way. @cbovar I just didn't see your pr. Otherwise, I would have added some comments on what didn't work for me^^

@laffuste I have created a pull request to upgrade as well as @cbovar. There was an issue with the button click handlers as well which is fixed in mine but not in this one. Therefore, https://github.com/Joelius300/ChartJSBlazor/pull/75 will be fixing this issue because it's more complete.

@cbovar I don't know why you didn't see the onclick stuff on your side... Maybe it was my Resharper telling me to update something there? It worked at least (Clicking on the buttons in the demo added some new data points 😄) afterwards.