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

Automatic destroy all charts on turbo:before-render causing issues #133

Closed
ocher opened this issue Mar 14, 2022 · 5 comments
Closed

Automatic destroy all charts on turbo:before-render causing issues #133

ocher opened this issue Mar 14, 2022 · 5 comments

Comments

@ocher
Copy link

ocher commented Mar 14, 2022

Currently Chartkick.js automatically adds a listener which removes all Chartkick.js charts upon turbo:before-render event. Unfortunately it is causing issues when there is a turbo form with data-turbo-frame and data-turbo-action specified. The latter causes that turbo:before-render is fired, and even though only contents of a single frame were updated, charts on the whole page are destroyed.

Frankly speaking, there is a chance that it is a Turbo bug, but at least from our point of view it was easier to fix that on Chartkick.js side, by disabling the automatic turbo:before-render and adding a code which removes charts in a different way (see the comment).

Anyway, maybe it is worth to check it, or at least provide some easy option of disabling automatic turbo:before-render listener?

@ocher
Copy link
Author

ocher commented Mar 14, 2022

We are cleaning up not displayed charts using Stimulus. First we change the HTML template to use the stimulus controller like this:

Chartkick.options[:html] = %(<div id="%{id}" data-controller="chartkick-cleanup") +
  %(style="height: %{height}; width: %{width}; text-align: center;">%{loading}</div>)

And here is the stimulus controller defined in chartkick_cleanup_controller.js file:

import { Controller } from 'stimulus'

export default class extends Controller {
  disconnect() {
    const chartId = this.element.id;
    if (Chartkick.charts[chartId]) {
      Chartkick.charts[chartId].destroy();
      delete Chartkick.charts[chartId];
    }
  }
}

That approach is pretty simple and guarantees that charts which are no longer displayed are correctly destroyed.

@ankane
Copy link
Owner

ankane commented May 19, 2022

Hey @ocher, thanks for reporting! Unfortunately, I don't see a way to tell if the event will re-render the entire page or just a frame. Just pushed the ability to set:

Chartkick.configure({autoDestroy: false})

to the auto-destroy branch. Let me know if that works for you.

https://github.com/ankane/chartkick.js/compare/auto-destroy

@ocher
Copy link
Author

ocher commented May 19, 2022

Hi @ankane, thank you for the response. Yes, I think that such option will be very helpful. 👍🏻

@elalemanyo
Copy link

elalemanyo commented May 23, 2022

Hi,
I am also having some issues with chartkick inside turbo frame, charts are only loading after page reload 😞
I tried @ocher fix but didn't work for me 🤷‍♂️.
Also tried auto-destroy branch but after loading it I am getting strange errors from esbuild:
✘ [ERROR] Could not resolve "chartkick"
Any help would be great.

BTW I also tried this, using own stimulus controller to init all charts didn't make any difference, but I see the connect being called after turbo frame load. Even that charts are not being render... no idea why

@ankane
Copy link
Owner

ankane commented Jun 16, 2022

Sorry, meant to follow up. The autoDestroy option was released in 4.2.0. Thanks again for reporting @ocher!

@elalemanyo I'm not sure how to reproduce (and don't have the bandwidth to help with app-specific issues), but hopefully the option helps.

@ankane ankane closed this as completed Jun 16, 2022
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

No branches or pull requests

3 participants