chartjs / chartjs-plugin-deferred

Chart.js plugin to defer initial chart updates
https://chartjs-plugin-deferred.netlify.app/
MIT License
108 stars 21 forks source link

Add method to remove event listeners/cleanup #4

Closed powerbuoy closed 7 years ago

powerbuoy commented 7 years ago

I really don't mean to spam you :)

But while using the deferred plugin on my Aurelia SPA I've noticed that unless you view every chart on a page (at which point the plugin unwatches the chart's scroll event https://github.com/chartjs/chartjs-plugin-deferred/blob/master/src/plugin.js#L107); if you go to another page and scroll down you'll get Uncaught TypeError: Cannot read property 'offsetParent' of null from chartInViewport() (https://github.com/chartjs/chartjs-plugin-deferred/blob/master/src/plugin.js#L53 I believe).

I might be missing something, but is there a way to manually remove the events? That way I can do so in my ViewModel's detached() (or React's componentWillUnmount()) similarly to how I can destroy the ChartJS instance with chart.destroy()?

Or perhaps there's a way for the plugin itself to detect calls to chart.destroy() and clean up after itself?

Again, sorry if I simply missed this feature or am using the plugin incorrectly, all I've done is import it and add the deferred options to my chart.

simonbrunel commented 7 years ago

You right, that's a bug, we need to stop monitoring a chart that has been destroyed. I think it can be fixed by unwatching in the destroy extension (line 203):

Chart.plugins.register({
    // ...

    beforeDatasetsUpdate: function(chart, options) {
        //...
    },

    destroy: function(chart) {
        unwatch(chart);
    }
});
powerbuoy commented 7 years ago

That sounds really promising. Is this something you could release along with import support?

simonbrunel commented 7 years ago

Sure, did you get a chance to test this fix?

powerbuoy commented 7 years ago

It does indeed work :) Nice job. Any ETA on a new version?

simonbrunel commented 7 years ago

Fixed in 8b4dacfa1193. @powerbuoy if you can confirm that this is fixed in master, I can trigger a new 0.3 release very soon (version 1.0 will contain breaking changes and should be released after Chart.js 2.6 is out).

powerbuoy commented 7 years ago

Works perfectly :)

simonbrunel commented 7 years ago

Released in v0.3.0