Fix teardown process

[serradura]

Whats issues this PR fixes?

1. Fix the listener removal process.

2. Fix the teardown process when the cache is disabled.

Final result

[excid3]

Thanks for the PR @serradura! 👍

Gonna review this afternoon, but it looks great from a first glance.

[serradura]

@jeffreyguenther done! Thanks for the revision.

[excid3]

@serradura You might know more about this than I do, but could this also be fixed if we used say the turbolinks:visit event in both cases?

It seems like that could work and would simplify things so that we only have to handle it in one way. We originally went with before-cache simply because I was thinking about the caching problem when we built this in the first place but that may not be the best one.

[serradura]

@excid3 the short answer is yes! But I recommend using the turbolinks:before-render to await the request end.

The long answer...

Turbolinks fires its events in the following order:

When a visit happens.

turbolinks:click
turbolinks:before-visit
turbolinks:request-start
turbolinks:visit
turbolinks:request-end
turbolinks:before-cache
turbolinks:before-render
turbolinks:render
turbolinks:load

When the history back button is used.

turbolinks:visit
turbolinks:before-cache
turbolinks:before-render
turbolinks:render
turbolinks:load

Note: If the cache is disabled the turbolinks:before-cache event will be skipped.

So, to simplify the things we could use turbolinks:visit or turbolinks:before-render. But as I said, I recommend turbolinks:before-render.

If we agree about the event to apply, the final script version will be this:

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

var TurbolinksAdapter = {
  beforeMount: function() {
    if (this.$el.parentNode) {
      handleVueDestruction( this )
      this.$originalEl = this.$el.outerHTML;
    }
  },
  destroyed: function() {
    this.$el.outerHTML = this.$originalEl;
  }
};

export default TurbolinksAdapter;

Because of your question I saw thats my previous solution had an error. The turbolinks:before-visit is not fired when the back button/history is used.

[excid3]

Yeah I think you're right. before-render should be the very last moment each request that we could handle things. I didn't realize that before-cache wasn't called every time but it makes perfect sense that it isn't when caching is disabled.

I'd vote to change that over to use before-render in both cases so that we have a consistent event being used every time and we don't introduce any extra logic. It'll certainly make debugging other things in the future a lot simpler that way.

What do you guys think?

[serradura]

@excid3 @jeffreyguenther done! I applied the before-render. 😉

[excid3]

Just published to npm. Thanks a bunch @serradura! 🍻

[excid3]

You know what, I think we need to use that turbolinks:visit event because this needs to run before caching (if enabled). Otherwise if we run it on the before-render we're going to end up with it caching the old Vue HTML in the previous event (before-cache).

[jeffreyguenther]

Interesting! I follow what you're saying. I wonder if #7 will be addressed if we solve this problem.

[serradura]

@excid3 first of all the before-render will be executed when the real and cached view version be rendered.

The turbolinks:render item in https://github.com/turbolinks/turbolinks#full-list-of-events section explain this behaviour.

But I had an eureka moment and I make new versions of the mixin:

Iteration 1

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

  var TurbolinksAdapter = {
    beforeMount: function() {
      if (this.$el.parentNode) {
        handleVueDestruction( this, 'turbolinks:before-render' )
        this.$originalEl = this.$el.outerHTML;
      }
    },
    destroyed: function() {
      this.$el.outerHTML = this.$originalEl;
    }
  };

Iteration 2

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

  function buildTurbolinksAdapterTo(destroyVue) {
    return {
      beforeMount: function() {
        if (this.$el.parentNode) {
          destroyVue(this);
          this.$originalEl = this.$el.outerHTML;
        }
      },
      destroyed: function() {
        this.$el.outerHTML = this.$originalEl;
      }
    };
  }

  var destroyVueOnBeforeRender = handleVueDestructionOn(
    'turbolinks:before-render'
  );

  var TurbolinksAdapter = buildTurbolinksAdapterTo(
    destroyVueOnBeforeRender
  )

Iteration 3

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

  function buildTurbolinksAdapterTo(destroyVue) {
    return {
      beforeMount: function() {
        if (this.$el.parentNode) {
          destroyVue(this);
          this.$originalEl = this.$el.outerHTML;
        }
      },
      destroyed: function() {
        this.$el.outerHTML = this.$originalEl;
      }
    };
  }

  var destroyVueOnBeforeRender = handleVueDestructionOn(
    'turbolinks:before-render'
  );

  var destroyVueOnBeforeCache = handleVueDestructionOn(
    'turbolinks:before-cache'
  );

  var destroyVueOnVisit = handleVueDestructionOn(
    'turbolinks:visit'
  );

  function isTurbolinksCacheDisabled() {
    var meta = document.head.querySelector('[name="turbolinks-cache-control"]');

    return meta && meta.content === 'no-cache';
  }

  var TurbolinksAdapter = buildTurbolinksAdapterTo(
    function(vue) {
      var destroy = isTurbolinksCacheDisabled()
                    ? destroyVueOnBeforeRender
                    : destroyVueOnBeforeCache;
      destroy(vue);
    }
  );

  TurbolinksAdapter.toDestroy = {
    onVisit: buildTurbolinksAdapterTo(destroyVueOnVisit),
    onBeforeCache: buildTurbolinksAdapterTo(destroyVueOnBeforeCache),
    onBeforeRender: buildTurbolinksAdapterTo(destroyVueOnBeforeRender)
  };

Highlights of the third iteration:

1. By default we delivery a smarter solution via TurbolinksAdapter.

2. We'll allow the developers choose the best strategy for its components' requirements. e.g: Using standalone strategies.

   new Vue({
     el,
     data: {
       message: 'Hello Vue!'
     },
     computed: {
       dynamicMessage: function() {
         return `${ this.message } ${ Date.now() }`;
       }
     },
     mixins: [TurbolinksAdapter.toDestroy.onVisit]
   });

What do you think? Too much? 😅

cc: @jeffreyguenther

[jeffreyguenther]

@serradura ya, probably a little too much. I think using turbolinks:visit works well. I've been testing it.

@excid3 Using turbolinks:visit also seems to address #7. I've done a quick test with bindings and as far as I can tell things work as we intend. That said, I would like someone to take a second look.

[excid3]

Using visit will work correctly in both cases. I went ahead and published that change.

I did confirm that before-render does not work correctly if your element and template don't match up exactly (which was the original problem for this package to solve). For example

<div id="hello"></div>

  var vueapp = new Vue({
    el: "#hello",
    template: '<p>Hello</p>',
 })

This is why we need to make sure the teardown happens before the caching so that we can make sure the #hello div is the one cached, and not the p tag from Vue so it can properly re-mount next view.

visit will work in all cases so we can both keep the package simple but also handle both situations with caching. With @serradura's fix for removing the teardown listener, it looks like it's working great now.

[jeffreyguenther]

Great! I accidentally pushed a commit with the exact same change. Didn't realize you had already done it. I'll delete those two commits from the repo.

[serradura]

@excid3 @jeffreyguenther good to know about before-render issue. This thread has been helping me to achieve a better understanding about Turbolinks.

So, turbolinks:visit is the right choice to be the default solution.

But about the implementations, let's go with the option 1 or 2?

Option 1

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

var TurbolinksAdapter = {
  beforeMount: function() {
    if (this.$el.parentNode) {
      handleVueDestructionOn('turbolinks:visit', this);
      this.$originalEl = this.$el.outerHTML;
    }
  },
  destroyed: function() {
    this.$el.outerHTML = this.$originalEl;
  }
};

Option 2

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

function buildTurbolinksAdapterTo(destroyVue) {
  return {
    beforeMount: function() {
      if (this.$el.parentNode) {
        destroyVue(this);
        this.$originalEl = this.$el.outerHTML;
      }
    },
    destroyed: function() {
      this.$el.outerHTML = this.$originalEl;
    }
  };
}

var destroyVueOnBeforeRender = handleVueDestructionOn(
  'turbolinks:before-render'
);

var destroyVueOnBeforeCache = handleVueDestructionOn(
  'turbolinks:before-cache'
);

var destroyVueOnVisit = handleVueDestructionOn(
  'turbolinks:visit'
);

var TurbolinksAdapter = buildTurbolinksAdapterTo(
  destroyVueOnVisit
);

TurbolinksAdapter.toDestroy = {
  onBeforeRender: buildTurbolinksAdapterTo(destroyVueOnBeforeRender),
  onBeforeCache: buildTurbolinksAdapterTo(destroyVueOnBeforeCache),
  onVisit: TurbolinksAdapter
};

[excid3]

I don't know that we need to make that even customizable because visit should work in all cases. if we discover that isn't the case we can probably take one of these approaches later on. For now though I think that what we've got is good.