jeffreyguenther / vue-turbolinks

A Vue mixin to fix Turbolinks caching
MIT License
287 stars 20 forks source link

Use turbolinks:before-cache event instead of turbolinks:visit #16

Closed printercu closed 6 years ago

printercu commented 6 years ago

Hi!

Have you tried this already? If there's no known issues, I think before-cache is better because components would be alive and not removed from current page until new page is loaded.

Using turbolinks:visit with slow internet connection, components disappear from page before new page is loaded.

jeffreyguenther commented 6 years ago

We used to listen for before-cache, you can read about the thinking that lead to the change in #8.

printercu commented 6 years ago

Ok, but there is issue with disappearing/blinking components on long pageloads/slow internet. You can try any of you existing app with enabled throttling on Network tab in chrome devtools. When you leave page with component, it returns back to initial state for a second or so before new page gets loaded. I've fixed it in my gist from #15: 2 last revisions in https://gist.github.com/printercu/e202851c370ffc38bcdedeb8300d417e/revisions

UPD. blinking is especially noticeable with script-tag templates, as component's initial content is empty.

nicklozon commented 6 years ago

I'm just learning about Vue.js/Rails integration and definitely noticing the flickering in my local development environment when using vue-turbolinks.

I could PR the ability to provide an options parameter for setting your own listener.

Edit: Nevermind. After reviewing a good chunk of Issue discussions on this repo it seems other events have their own issues and turbolinks:visit is the vest, although it does cause some flickering issues.

jeffreyguenther commented 6 years ago

Ya, unfortunately, we haven't found a perfect solution yet. It's tricky to get the two techniques to play nice with each other.

I haven't had the time to do this, but it might be worth investigating if we can issue a change to Turbolinks to allow us to have a better spot in the page's lifecycle to do this work.

printercu commented 6 years ago

I've re-read again linked issue, and understood that before-cache is not used because it's not fired when cache is disabled. I don't think it's a good solution to make all apps blinking, only because some others may disable cache. I see couple of other solutions:

jeffreyguenther commented 6 years ago

I'm game to review a PR that handles this complexity for users. I'm interested in an approach that does-the-right-thing:tm: with as little effort from the user as possible, while at the same time as providing an escape hatch if you want to do things on your own. @excid3 what do you think?

excid3 commented 6 years ago

The default should be the most compatible event (probably what we currently have), but we can make it so you can override it if you prefer to have it run on another event. I'm fine with making it customizable.

nicklozon commented 6 years ago

I've created a commit (not pushed yet) that allows for an options parameter to be provided so that the event can be overridden. I can push when I get home.

The unfortunate thing is that I still find the page flickers even when using before-cache.

excid3 commented 6 years ago

You're probably always going to get a flicker because it has to be unmounted and remounted every time.

The only way you can get by without a flicker is to use the same ID inside your Vue template so that the cache can store the old rendered copy of the Vue app and next pageview it can replace it. You wouldn't need this library to do that.

nicklozon commented 6 years ago

FYI: https://github.com/nicklozon/vue-turbolinks/commit/de9bbe0f8966324a101ba2336476079cb48f994a

Usage would be:

Vue.use(TurbolinksAdapter, { event: 'turbolinks:before-cache' })
nicklozon commented 6 years ago

My "flickering" issue stems from placing my template code directly in my rails erb files. This causes turbolinks to render the template before vue has done it's work.

If I instead use a vue file and the render option on the Vue object, this means I won't see the "flickering", instead I will see an empty page until vue renders the template into the div.

Update: Vue has a v-cloak tag which will actually hide the template until vue is done rendering. Related blog post here.

printercu commented 6 years ago

@nicklozon can you please give a try for https://gist.github.com/printercu/e202851c370ffc38bcdedeb8300d417e It should work fine both for .vue files and <script type='x-template'>. I don't use div templates because they have some limitations/edgecases and can show template source on page while page load, so have not tested for them.

Compiled coffee => js ```js // Inspired by // https://github.com/jeffreyguenther/vue-turbolinks/blob/master/index.js // but changed to support going back to cached page with vue instance. // Original version will keep instance created from cache along with new one. var plugin; plugin = { instances: [], bind: function() { // Destroy instances on current page when moving to another. document.addEventListener('turbolinks:before-cache', function() { return plugin.cleanupInstances(); }); // Destroy left instances when previous page has disabled caching. document.addEventListener('turbolinks:before-render', function() { return plugin.cleanupInstances(); }); // Clear instances on curent page which are not present anymore. return document.addEventListener('turbolinks:load', function() { return plugin.cleanupInstances(function(x) { return document.contains(x.$el); }); }); }, cleanupInstances: function(keep_if) { var i, instance, len, ref, result; result = []; ref = plugin.instances; for (i = 0, len = ref.length; i < len; i++) { instance = ref[i]; if (typeof keep_if === "function" ? keep_if(instance) : void 0) { result.push(instance); } else { instance.$destroy(); } } return plugin.instances = result; }, Mixin: { beforeMount: function() { // If this is the root component, // we want to cache the original element contents to replace later. // We don't care about sub-instances, just the root if (this === this.$root) { plugin.instances.push(this); return this.$originalEl = this.$el.outerHTML; } }, destroyed: function() { if (this === this.$root) { // We only need to revert the html for the root component. return this.$el.outerHTML = this.$originalEl; } } }, install: function(Vue, _options) { plugin.bind(); return Vue.mixin(plugin.Mixin); } }; // export default plugin // or: // Vue.TurbolinksAdapter = plugin // plugin.bind() ```
adrianthedev commented 6 years ago

this works wonderfully!

I would definitely tweak the repo code to work like this.

Thanks a lot @printercu

LostKobrakai commented 6 years ago

How about just this? Use whatever comes first:

function handleVueDestructionOn(vue) {
  function teardown() {
    vue.$destroy();
    document.removeEventListener('turbolinks:before-cache', teardown);
    document.removeEventListener('turbolinks:before-render', teardown);
  }
  document.addEventListener('turbolinks:before-cache', teardown);
  document.addEventListener('turbolinks:before-render', teardown);
}

[…]
  beforeMount () {
      // If this is the root component, we want to cache the original element contents to replace later
      // We don't care about sub-components, just the root
      if (this !== this.$root) return
      handleVueDestructionOn(this)
      this.$originalEl = this.$el.outerHTML
    },
[…]
connorshea commented 5 years ago

So I had this problem and it was caused by the same thing as @nicklozon, where I had template code inside my .html.erb file.

<div id="game-library">
  <library
    v-bind:purchased-releases="<%= @purchased_releases.to_json(except: [:created_at, :updated_at]) %>"
  ></library>
</div>

And then in library.js, which I imported into my application.js:

import TurbolinksAdapter from 'vue-turbolinks';
import Vue from 'vue/dist/vue.esm';
import Library from './components/library.vue';

Vue.use(TurbolinksAdapter)

document.addEventListener('turbolinks:load', () => {
  let gameLibrary = document.getElementById("game-library")
  if (gameLibrary != null) {
    const library = new Vue({
      el: '#game-library',
      components: { Library }
    })
  }
});

^^^ the above is what it looks like when the problem exists. ^^^

@printercu's script (this) worked for me, though it does seem like - based on nick's comment previously - you can also avoid this problem by simply not including the Vue.js custom elements in the .html.erb files, e.g. by plugging it into a data- attribute and then mounting it on that, or something?