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

createChart is not fired after Turbo Frame renders the view #608

Open
tbcooney opened this issue Mar 27, 2023 · 11 comments · May be fixed by #610
Open

createChart is not fired after Turbo Frame renders the view #608

tbcooney opened this issue Mar 27, 2023 · 11 comments · May be fixed by #610

Comments

@tbcooney
Copy link

tbcooney commented Mar 27, 2023

Describe the bug
The Chartkick graph is not rendered when the Turbo Frame is replaced using Turbo Frames with GET requests. Others have reported the same issue before.

To reproduce
Steps to reproduce.

<div class="flex justify-end mb-1">
  <%= form_with url: [:reports, :sales], method: :get, data: { turbo_frame: "sales", turbo_action: "advance" } do |form| %>
    <%= form.date_field :start_date, value: params[:start_date], class: "form-input" %>
    <%= form.date_field :end_date, value: params[:end_date], class: "form-input" %>
  <% end %>
</div>
<%= turbo_frame_tag "sales" do %>
  <%= column_chart @sales %>
  <!-- Sales table -->
<% end %>

The form above uses a data-turbo-frame to target the sales frame and tells Turbo to use the response from the form submission to replace the content of the sales frame. The page URL is also updated by adding a turbo-action data attribute to the form that triggers the frame navigation.

After submitting the form, the chart body appears but the <script> function is not executed so the chart is not displayed.
Checking the window.Chartkick.charts object returns {}.

HTML generated by Chartkick

<turbo-frame id="sales" src="http://lvh.me:5000/reports/sales?start_date=2022-11-01&end_date=2022-11-04" complete="">
  <div id="chart-1" style="height: 280px; width: 100%; text-align: center; color: #999; line-height: 280px; font-size: 14px; font-family: 'Lucida Grande', 'Lucida Sans Unicode', Verdana, Arial, Helvetica, sans-serif;"><canvas style="box-sizing: border-box;" width="300"></canvas></div>
  <script>
    (function() {
      if (document.documentElement.hasAttribute("data-turbolinks-preview")) return;
      if (document.documentElement.hasAttribute("data-turbo-preview")) return;

      var createChart = function() { new Chartkick["ColumnChart"]("chart-1", [["2022-11-01","0"],["2022-11-02","0"],["2022-11-03","0"],["2022-11-04","0"]], {"legend":false,"colors":["#000","#000"],"prefix":"$","round":2,"zeros":true,"thousands":","}); };
      if ("Chartkick" in window) {
        createChart();
      } else {
        window.addEventListener("chartkick:load", createChart, true);
      }
    })();
  </script>
</turbo-frame>

As part of the PR from hotwired/turbo that triggers the frame navigation and allows for this type of form submission to occur, Turbo will dispatch a turbo:load event.

A simple fix that results in the expected behaviour was to add an event listener for the turbo:load event, and call createChart within that event listener.

--- a/chartkick/lib/helper.rb
+++ b/chartkick/lib/helper.rb
js = <<~JS
  <script#{nonce_html}>
    (function() {
      if (document.documentElement.hasAttribute("data-turbolinks-preview")) return;
      if (document.documentElement.hasAttribute("data-turbo-preview")) return;

      var createChart = function() { #{createjs} };
      if ("Chartkick" in window) {
        createChart();
+
+       window.addEventListener("turbo:load", createChart);
      } else {
        window.addEventListener("chartkick:load", createChart);
      }
    })();
  </script>
JS
@tbcooney
Copy link
Author

tbcooney commented Mar 27, 2023

When building search forms (or reporting forms with search capabilities), you’ll likely use a similar approach. @ankane I think that updating the library to wait for the turbo:load event and allowing for Turbo to process the rendered HTML and update the DOM will alleviate any frame replacement errors like this one.

@jedbarlow
Copy link

I'm experiencing what seems like same issue. The charts are not loading when navigating to a page for the first time with turbo. The proposed fix makes the charts load as expected. However, subsequent navigation in the app then results in error messages in the console, since the listeners on turbo:load are still active but turbo has loaded a new page that does not contain those charts. Adding the once: true option prevents these error messages in my case while still solving the original problem of the charts failing to load.

window.addEventListener("turbo:load", createChart, {once: true});

@DanielJackson-Oslo
Copy link

Also running into this. Any way to fix it locally, while we wait for a fix?

@tbcooney
Copy link
Author

Also running into this. Any way to fix it locally, while we wait for a fix?

You can override helper.rb with the fix above.

@DanielJackson-Oslo
Copy link

Also running into this. Any way to fix it locally, while we wait for a fix?

You can override helper.rb with the fix above.

Thanks!

As far as I can tell I then need to override the whole function, which is quite long? (New to Rails, so asking stupid questions left and right!)

I did it using the following file content, but it definitely is very fragile, as any changes in any of the other 90 lines of code that I didn't want to change will introduce weird bugs.

module ChartkickHelper

    # TODO: THIS IS A HACK to make turbo frames play nicely with charts
    #  ONLY KEEP UNTIL THE FOLLOWING BUG IS FIXED: https://github.com/ankane/chartkick/issues/608
    #  IT CAN BE SAFELY DELETED WHEN THAT BUG IS FIXED
    def chartkick_chart(klass, data_source, **options)
      options = Chartkick::Utils.deep_merge(Chartkick.options, options)

      @chartkick_chart_id ||= 0
      element_id = options.delete(:id) || "chart-#{@chartkick_chart_id += 1}"

      height = (options.delete(:height) || "300px").to_s
      width = (options.delete(:width) || "100%").to_s
      defer = !!options.delete(:defer)

      # content_for: nil must override default
      content_for = options.key?(:content_for) ? options.delete(:content_for) : Chartkick.content_for

      nonce = options.fetch(:nonce, true)
      options.delete(:nonce)
      if nonce == true
        # Secure Headers also defines content_security_policy_nonce but it takes an argument
        # Rails 5.2 overrides this method, but earlier versions do not
        if respond_to?(:content_security_policy_nonce) && (content_security_policy_nonce rescue nil)
          # Rails 5.2+
          nonce = content_security_policy_nonce
        elsif respond_to?(:content_security_policy_script_nonce)
          # Secure Headers
          nonce = content_security_policy_script_nonce
        else
          nonce = nil
        end
      end
      nonce_html = nonce ? " nonce=\"#{ERB::Util.html_escape(nonce)}\"" : nil

      # html vars
      html_vars = {
        id: element_id,
        height: height,
        width: width,
        # don't delete loading option since it needs to be passed to JS
        loading: options[:loading] || "Loading..."
      }

      [:height, :width].each do |k|
        # limit to alphanumeric and % for simplicity
        # this prevents things like calc() but safety is the priority
        # dot does not need escaped in square brackets
        raise ArgumentError, "Invalid #{k}" unless html_vars[k] =~ /\A[a-zA-Z0-9%.]*\z/
      end

      html_vars.each_key do |k|
        # escape all variables
        # we already limit height and width above, but escape for safety as fail-safe
        # to prevent XSS injection in worse-case scenario
        html_vars[k] = ERB::Util.html_escape(html_vars[k])
      end

      html = (options.delete(:html) || %(<div id="%{id}" style="height: %{height}; width: %{width}; text-align: center; color: #999; line-height: %{height}; font-size: 14px; font-family: 'Lucida Grande', 'Lucida Sans Unicode', Verdana, Arial, Helvetica, sans-serif;">%{loading}</div>)) % html_vars

      # js vars
      js_vars = {
        type: klass.to_json,
        id: element_id.to_json,
        data: data_source.respond_to?(:chart_json) ? data_source.chart_json : data_source.to_json,
        options: options.to_json
      }
      js_vars.each_key do |k|
        js_vars[k] = Chartkick::Utils.json_escape(js_vars[k])
      end
      createjs = "new Chartkick[%{type}](%{id}, %{data}, %{options});" % js_vars

      warn "[chartkick] The defer option is no longer needed and can be removed" if defer

      # Turbolinks preview restores the DOM except for painted <canvas>
      # since it uses cloneNode(true) - https://developer.mozilla.org/en-US/docs/Web/API/Node/
      #
      # don't rerun JS on preview to prevent
      # 1. animation
      # 2. loading data from URL
      js = <<~JS
        <script#{nonce_html}>
          (function() {
            if (document.documentElement.hasAttribute("data-turbolinks-preview")) return;
            if (document.documentElement.hasAttribute("data-turbo-preview")) return;

            var createChart = function() { #{createjs} };
            if ("Chartkick" in window) {
              window.addEventListener("turbo:load", createChart, {once: true});
            } else {
              window.addEventListener("chartkick:load", createChart, true);
            }
          })();
        </script>
      JS

      if content_for
        content_for(content_for) { js.respond_to?(:html_safe) ? js.html_safe : js }
      else
        html += "\n#{js}"
      end

      html.respond_to?(:html_safe) ? html.html_safe : html
    end
end

@wilg
Copy link

wilg commented Aug 1, 2023

I opened a PR #610 with the fix from above. If you wish to use it, just update your Gemfile to:

gem "chartkick", git: "https://github.com/wilg/chartkick", branch: "wilg/turbo"

@BroiSatse
Copy link

Just hit the same issue - the problem seems to be caused by chartkick destroying all charts on turbo:before-render. It seems that inline script runs before that hooks, so new charts are destroyed just after they are rendered.

I fixed it using the following:

Chartkick.config.autoDestroy = false

window.addEventListener('turbo:before-render', () => {
  Chartkick.eachChart(chart => {
    if (!chart.element.isConnected) {
      chart.destroy()
      delete Chartkick.charts[chart.element.id]
    }
  })
})

@forsbergplustwo
Copy link

Note to anyone else copying @DanielJackson-Oslo code above, the class/module has changed from:

module ChartkickHelper
...
end

to:

class Chartkick
  module Helper
    ...
  end
end

Can confirm works as of days date when using TurboDrive and TurboFrames 🎉

@idev4u
Copy link

idev4u commented Jul 16, 2024

hi folks, I run in the same issue and I don't like to say that, but I have no idea how it works, but this solve the issue for me.

I add javascript include tag in app/views/layouts/application.html.erb

...
     <%= stylesheet_link_tag "application", "data-turbo-track": "reload" %>
     <%= javascript_importmap_tags %>
     <%= javascript_include_tag "//www.google.com/jsapi", "chartkick" %>
   </head>
...

to be fair got this from
https://stackoverflow.com/questions/73452250/chartkick-not-loading-in-ruby-on-rails-7

@ankane
Copy link
Owner

ankane commented Nov 13, 2024

Hi all, I'm having trouble reproducing the issue (using the latest version of Turbo).

I've created a fresh Rails 8 app (with a minimal setup of Chartkick). If someone is able to create a minimal PR to reproduce, happy to look into it more.

@forsbergplustwo
Copy link

Thank you @ankane - For what it's worth, I was not able to reproduce using the fresh Rails 8 app either. Tried all kinds of combinations with frames, streams etc.

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 a pull request may close this issue.

8 participants