mariusmuntean / ChartJs.Blazor

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

OnClick callback does not work, it throws an exception in the frontend #54

Closed Arrowana closed 4 years ago

Arrowana commented 4 years ago

Describe the bug

Pretty much the title

Which Blazor project type is your bug related to?

Which charts does this bug apply to?

ChartJsLineChart but i haven't tried other

To Reproduce

Steps to reproduce the behavior:

  1. Setup a OnClick callback on the chart LineConfig.LineOptions
  2. Go to the web browser (I tried Firefox and Chrome) and click on the chart
  3. A javascript error is thrown in the frontend:
TypeError: n.onClick.call is not a function 17 Chart.min.js:7:101051
    handleEvent https://localhost:5001/_content/ChartJs.Blazor/Chart.min.js:7
    eventHandler https://localhost:5001/_content/ChartJs.Blazor/Chart.min.js:7
    i https://localhost:5001/_content/ChartJs.Blazor/Chart.min.js:7
    <anonymous> https://localhost:5001/_content/ChartJs.Blazor/Chart.min.js:7

Expected behavior

OnClick callback should be called without throwing an exception

Joelius300 commented 4 years ago

Thank you for the report!

Could you show us the code you wrote which sets up the OnClick callback? The more code we have to write ourself and can't copy from you, the harder it is to properly reproduce the problem. This doesn't mean that you need to show us all of your code but I think the code of interest would be the problematic razor-page including the setup of the OnClick function and maybe also how you include the necessary scripts in _Host.cshtml.

db85 commented 4 years ago

I get exactly the same error message. The problem is probably because I load the data async inside OnInitializedAsync().

To reproduce this I modified the OnClickHandler example:

                await Task.Delay(100);

                var doughnutSet = new PieDataset
                {
                    BackgroundColor = new[] {ColorUtil.RandomColorString(), ColorUtil.RandomColorString(), ColorUtil.RandomColorString(), ColorUtil.RandomColorString()},
                    BorderWidth = 0,
                    HoverBackgroundColor = ColorUtil.RandomColorString(),
                    HoverBorderColor = ColorUtil.RandomColorString(),
                    HoverBorderWidth = 1,
                    BorderColor = "#ffffff"
                };

                doughnutSet.Data.AddRange(new double[] {4, 5, 6, 7});
                _config.Data.Datasets.Add(doughnutSet);

                _doughnutChartJs.Update();

Is there a solution?

mariusmuntean commented 4 years ago

@db85 I will look into this more closely tomorrow.

Until then I suspect it has to do with the call _doughnutChartJs.Update(); which shouldn't be necessary in OnInitializedAsync() since the chart is updated after it is rendered (see here: https://github.com/mariusmuntean/ChartJs.Blazor/blob/master/src/ChartJs.Blazor/Charts/ChartBase.cs)

But for now I'm just speculating 😅

Arrowana commented 4 years ago

Actually i didn't reply because when trying to reproduce i realised this strange behavior related to chart.Update(). So i started to believe it was my fault somehow.

db85 commented 4 years ago

@mariusmuntean I don't see any data without _doughnutChartJs.Update();. But yes, the problem is the Update.

//ChartJsInterop.ts
    public UpdateChart(config: ChartConfiguration): boolean {
        if (!this.BlazorCharts.has(config.canvasId))
            throw `Could not find a chart with the given id. ${config.canvasId}`;

        let myChart = this.BlazorCharts.get(config.canvasId);

        /// Handle datasets
        this.HandleDatasets(myChart, config);

        /// Handle labels
        this.MergeLabels(myChart, config);

        // Handle options - mutating the Options seems better because the rest of the computed options members are preserved
        Object.entries(config.options).forEach(e => {
            myChart.config.options[e[0]] = e[1];
        });

        myChart.update();
        return true;
    }

If I comment out the following then the example from above works

        // Handle options - mutating the Options seems better because the rest of the computed options members are preserved
        Object.entries(config.options).forEach(e => {
            myChart.config.options[e[0]] = e[1];
        });
mariusmuntean commented 4 years ago

So I looked into this and found the issue. Basically, the UpdateChart() function from ChartJsInterop.ts was incomplete. During an update, the new config is applied on top of the existing config, which broke the "wiring up" that is done during SetupChart which is necessary to invoke .Net methods as OnClick/OnHover handlers.

This bug is only triggered when you register .Net handlers for onClick/onHover and then you call Update() on the chart.

@db85 Calling _doughnutChartJs.Update() in OnInitialized() is dangerous if you consider the component's lifecycle. The very first time there should be a NullPointerException because the @ref directive doesn't populate the _doughnutChartJs variable until later (after the component is rendered). Due to prerendering, the lifecycle methods are called twice; the second time over there's no more NPE and the call to Update() goes through, but then it triggers the bug.

At least this is my current understanding of the issue right now.

To cut a long story short, I have a working version of a fix on this branch: https://github.com/mariusmuntean/ChartJs.Blazor/tree/IssueFix/54-OnClickCallback I expect to have a new release ready during the weekend. For anyone who cannot wait until then, a workaround would be to take the ChartJsBlazorInterop.js from my IssueFix branch, add it to their wwwroot folder and reference that in their index.html/_Host.cshtml instead of the one provided by my library (I mean the one from _content).

db85 commented 4 years ago

@mariusmuntean Thank you very much for the quick fix and the detailed answer. I have checked out your branch and it works. :+1:

mariusmuntean commented 4 years ago

I merged the fix into master