Error in destroyed hook when closing vuetify v-dialog

[tleish]

Found a possible bug with vue-turbolinks 2.0.4. When using it in conjunction with the vuetify library, it raises an error when closing a dialog/modal window.

[Vue warn]: Error in destroyed hook: 
  "NoModificationAllowedError: Failed to set the 'outerHTML' property on 'Element': 
   This element has no parent node."
DOMException: Failed to set the 'outerHTML' property on 'Element': 
  This element has no parent node. at VueComponent.destroyed

As it turns out, when you click a button to show the dialog, the beforeMount method from this plugin is called. In beforeMount, this and this.$root references the new dialog component. this.$originalEl is never set. this.$el is undefined as the dialog Dom has not yet been created. I believe the Dom created after.

Once the dialog IS mounted, this.$el is defined and references the overlay Dom (this.$el === div.v-overlay). When closing the dialog the app calls the vue-turbolinks destroy method and the conditional in that method evaluates to true, the method then calls this.$el.outerHTML = this.$originalEl. This is equivalent to this.$el.outerHTML = undefined since the beforeMount never defined the variable this.$originalEl. Hence the error "Failed to set the 'outerHTML' ".

Demo app reproducing the error: https://codepen.io/tleish/pen/MWwWLYa. Open a javascript console, click the button and close the dialog box a few times.

One possible fix might be:

    destroyed: function destroyed() {
      // We only need to revert the html for the root component
-     if (this == this.$root && this.$el) {
+     if (this == this.$root && this.$el && this.$originalEl) {
        this.$el.outerHTML = this.$originalEl;
      }
    }

Note: Similar, but slight different to Issue #17.

[tleish]

FYI, added codepen example to prior comment to make reproduction of bug easier.

[excid3]

Solid debugging. 👍

That fix seems like a reasonable to me. I don't think it should cause any issues with existing apps since it's simply verifying there is a cached element from setup. Do you see any downsides?

[tleish]

I can't think of any downsides.

Would you also want to export the mixin as separate option? Something like:

function handleVueDestructionOn(turbolinksEvent, vue) {
  document.addEventListener(turbolinksEvent, function teardown() {
    vue.$destroy();
    document.removeEventListener(turbolinksEvent, teardown);
  });
}

+ const turbolinksAdapterMixin = {
+   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-components, just the root
+     if (this == this.$root && this.$el) {
+       var destroyEvent = this.$options.turbolinksDestroyEvent || 'turbolinks:visit'
+       handleVueDestructionOn(destroyEvent, this);
+       this.$originalEl = this.$el.outerHTML;
+     }
+   },
+ 
+   destroyed: function() {
+     // We only need to revert the html for the root component
+     if (this == this.$root && this.$el && this.$originalEl) {
+       this.$el.outerHTML = this.$originalEl;
+     }
+   }
+ };

function plugin(Vue, options) {
  // Install a global mixin
-  Vue.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-components, just the root
-      if (this == this.$root && this.$el) {
-        var destroyEvent = this.$options.turbolinksDestroyEvent || 'turbolinks:visit'
-        handleVueDestructionOn(destroyEvent, this);
-        this.$originalEl = this.$el.outerHTML;
-      }
-    },
-
-    destroyed: function() {
-      // We only need to revert the html for the root component
-      if (this == this.$root && this.$el) {
-        this.$el.outerHTML = this.$originalEl;
-      }
-    }
-  })
+  Vue.mixin(turbolinksAdapterMixin)
};

+ export { turbolinksAdapterMixin };
export default plugin;

Use on all Vue components (same as example in README):

import TurbolinksAdapter from 'vue-turbolinks';
Vue.use(TurbolinksAdapter)

document.addEventListener('turbolinks:load', () => {
  var vueapp = new Vue({
    el: "#hello",
    template: '<App/>',
    components: { App }
  });
});

Use on specific (root) Vue component:

import { turbolinksAdapterMixin } from 'vue-turbolinks';

document.addEventListener('turbolinks:load', () => {
  var vueapp = new Vue({
    el: "#hello",
    mixins: [turbolinksAdapterMixin], // <-- add as mixin
    template: '<App/>',
    components: { App }
  });
});

[tleish]

Actually, a cleaner implementation would be to use the beforeMount to cache the html and register the destroyed hook only once for the root component. (see Programmatic Event Listeners — Vue.js).

- function handleVueDestructionOn(turbolinksEvent, vue) {
+ function handleVueDestruction(vue) {
+  var turbolinksEvent = vue.$options.turbolinksDestroyEvent || 'turbolinks:visit';
   document.addEventListener(turbolinksEvent, function teardown() {
     vue.$destroy();
     document.removeEventListener(turbolinksEvent, teardown);
   });
 }

 function plugin(Vue, options) {
   // Install a global mixin
   Vue.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-components, just the root
-      if (this == this.$root && this.$el) {
+      if (this === this.$root && this.$el) {
-        var destroyEvent = this.$options.turbolinksDestroyEvent || 'turbolinks:visit'
-        handleVueDestructionOn(destroyEvent, this);
+        handleVueDestruction(this);
         this.$originalEl = this.$el.outerHTML;

+        // Revert the html on destroy
+        this.$once('hook:destroyed', function() {
+          this.$el.outerHTML = this.$originalEl
+        });
       }
     },

-    destroyed: function() {
-      // We only need to revert the html for the root component
-      if (this == this.$root && this.$el) {
-        this.$el.outerHTML = this.$originalEl;
-      }
-    }
  })
};

export default plugin;

Final version of both this.$once and separate mixin export:

function handleVueDestruction(vue) {
  var turbolinksEvent = vue.$options.turbolinksDestroyEvent || 'turbolinks:visit';
  document.addEventListener(turbolinksEvent, function teardown() {
    vue.$destroy();
    document.removeEventListener(turbolinksEvent, teardown);
  });
}

var turbolinksAdapterMixin = {
  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-components, just the root
    if (this === this.$root && this.$el) {
      handleVueDestruction(this);
      // cache original element
      this.$originalEl = this.$el.outerHTML;
      // register root hook to restore original element on destroy
      this.$once('hook:destroyed', function() {
        this.$el.outerHTML = this.$originalEl
      });
    }
  }
};

function plugin(Vue, options) {
  // Install a global mixin
  Vue.mixin(turbolinksAdapterMixin)
};

export { turbolinksAdapterMixin };
export default plugin;

All prior conditionals still being met:

• this === this.$root
• this.$el

We no longer need to check for the existence of this.$originalEl, because we know the cache exists since it would have just been created.

With the prior implementation, beforeMount and destroyed are evaluated for every component. With this alternative implementation, beforeMount is still evaluated for every component, but destroyed is only evaluated for root components.

Also, exporting the mixin separately would no longer be need, although perhaps a good option for those who do not want the beforeMount evaluated with every component (my preference).

[excid3]

The $.once is a nice improvement! I'm wondering if this will be fully backwards compatible or not?

[tleish]

How backwards compatible do you hope to go? vm.$once('hook:destroyed', function() goes back to 1.0 (perhaps before). See: Vue.js 1.0 API

Working example: http://jsfiddle.net/tleish/fk40c8hv/15/

Note that beforeMount from the original script was not introduced until 2.0 (see: Vue.js 2.0 Changes). So the changes are backwards compatible to what they would have been before.

[excid3]

Oops, I didn't mean backwards compatible with older Vue. Just to make sure if we rewrote this it wouldn't break existing apps using vue-turbolinks.

Since this shouldn't break the old Vue.use and just adds the mixin export, I think it's good. 👍

I'm going to try and test this on one of my apps and make sure everything works, but do you want to make a PR and add some notes to the README about the new export option?