Fix turbolinks loading twice

[feliperaul]

On #536 you asked me to test the defert_turbolinks branch.

It kinda worked, but I noticed a flickering behavior on (it's very noticeable when you have something like library: { animation: { easing: 'easeOutQuad' }).

I'm not an expert on turbolinks, but have been working with it for quite a while, so I noticed the following on the code:

1. It was adding two event listeners, one on load and another one on turbolinks:load. That redundancy is not necessary and it was invoking the createChart function twice, since turbolinks always fires turbolinks:load on every document.ready, regardless if it's a first time visit or a visit due to an internal link click or a restoration visit (back browser button).

Maybe you added both because it didn't work like this on previous turbolinks versions (you had to listen for both events), but on Turbolinks 5 it works like this. Check out https://github.com/turbolinks/turbolinks#observing-navigation-events , where it states The most significant of these is the turbolinks:load event, which fires once on the initial page load, and again after every Turbolinks visit.

2. It was not cleaning up after itself. So I added a document.addEventListener("turbolinks:before-cache", cleanupBeforeCache) handler, which invokes chart.destroy(). This way the page is cached without the graphs already loaded, which I think should be the way to go, because it prevents the flickering behavior, memory leaks and the display of stale graph data even for a split second (Chartkick is very fast to load, I'd call it instant, so I don't think we're losing anything here).

This pull request has been tested on (1) visiting the graph pages directly and reloading them (so, first visit is on the page that contains Chartkick), (2) visiting the graph pages by loading a different page and then clicking on a link to load the page via turbolinks and (3) going back to the page with the graphs by using the back browser button (restoration visit), and it's working on all 3 scenarios :)

[ankane]

Hey @feliperaul, thanks for trying it out and the feedback.

I think there are two main cases to consider with Turbolinks:

1. Without defer: Chartkick currently works out of the box
2. With defer (this feature): Needed if application.js uses defer: true

From my testing, the double render happens due to Turbolinks’ preview feature in both cases. You can disable this with:

<meta name="turbolinks-cache-control" content="no-preview">

But I think Chartkick should likely follow the same behavior as other elements on the page if preview is not disabled. We may be able to get rid of the additional load listener, but I don’t think it’s having any effect currently since both are removed after one is run.

Clean up is a great point. It also affects both cases, so not sure it should be specific to the defer option. We probably want to do something in Chartkick.js

Edit:

• Explanation of double render: https://github.com/turbolinks/turbolinks/issues/167
• Also, on 2nd thought, not sure it makes sense to have Turbolinks-specific clean up code in the JS library. It might be best to just document how to clean up if desired.

[feliperaul]

@ankane I understand your points.

Yes, I agree that regardless of the defer option, as per https://github.com/turbolinks/turbolinks/issues/167#issuecomment-273795538 that you brought up, Turbolinks expects that all scripts should be idempotent, and for that to be achieved without that flickering I think Chartkick should cleanup after itself, destroying the chart elements before Turbolinks caching occur, like in this proposed pull request.

If we don't do that, I think we might even be facing a memory leak because of never cleaning up when the user navigates away from pages that contained a chart. From the docs in https://github.com/turbolinks/turbolinks:

In particular, you can no longer depend on a full page load to reset your environment every time you navigate. The JavaScript window and document objects retain their state across page changes, and any other objects you leave in memory will stay in memory.

I think you are better suited than me to make the cleanup happen in Chartkick.js if you want that cleanup to happen there. Please let me know if I can help in any other way!

[ankane]

To better summarize what I was trying to say above: I think the preview and clean up are both separate from the defer option, so don't think it makes sense to make the changes in the PR.

[feliperaul]

@ankane Got it, I'm closing the PR then.