Fix async switches

[BehindTheMath]

If any switches are async, the subsequent code will execute before the switches are finished. This commit moves all that code to a new function, and debounces the calls to onSwitch() so it only executes once, after all the switches finish.

Fizes #72.

[BehindTheMath]

@tremby What do you think?

[tremby]

Will read and respond tomorrow.

[robinnorth]

The concept of this PR is great and it solves a couple of issues I was having that were caused by onSwitch calling parseDOM multiple times before I was ready (I have an async switch and I'm using a custom switch method to dynamically opt certain links out of Pjax).

In testing with my project, however, I found that there's a slight logic flaw that needs correcting, which I've commented on inline. Once that's sorted though, this looks like it'll be really useful. Nice work!

[robinnorth]

Incrementing numPendingSwitches in this loop doesn't work -- as the switch functions are executed in the same loop, multiple synchronous switches will result in numPendingSwitches being incremented to 1 and then immediately decremented to 0 by the new code in onSwitch. This then results in afterAllSwitches being called more than once and throwing an error on the second run as at that point the state has been reset, resulting in this.state.options being null.

It would seem safe to instead assign numPendingSwitches before any looping, on the very first line of the method, like so:

module.exports = function(switches, switchesOptions, selectors, fromEl, toEl, options) {
  this.state.numPendingSwitches = selectors.length;

  selectors.forEach(function(selector) {
      // snip...

If one or more of the selectors aren't present in the switched document, the check made for each selector for newEls.length !== oldEls.length will throw an error and result in a full page reload being triggered.

I've tested this modification with my current project that has 3 synchronous and 1 async switches and it works for me, but obviously further testing would be required.

[BehindTheMath]

Good point.

[BehindTheMath]

Actually, it's more complicated than that, since it's not just one per selector, it's also times the number of newEls.

[robinnorth]

Good point 😉 You're quite right, I missed that.

Might need to loop selectors twice then: once to build the numPendingSwitches correctly, and once to fire off the switch callbacks. This works for me (I cache the querySelector operation results in a map to avoid unnecessary extra queries in the second loop):

var forEachEls = require("./foreach-els")

var defaultSwitches = require("./switches")

module.exports = function(switches, switchesOptions, selectors, fromEl, toEl, options) {
  var elsBySelectors = {};

  selectors.forEach(function(selector) {
    var newEls = fromEl.querySelectorAll(selector)
    var oldEls = toEl.querySelectorAll(selector)

    elsBySelectors[selector] = {
      newEls: newEls,
      oldEls: oldEls
    }

    this.state.numPendingSwitches += newEls.length
  }, this)

  selectors.forEach(function(selector) {
    var newEls = elsBySelectors[selector].newEls
    var oldEls = elsBySelectors[selector].oldEls
    if (this.log) {
      this.log("Pjax switch", selector, newEls, oldEls)
    }
    if (newEls.length !== oldEls.length) {
      // forEachEls(newEls, function(el) {
      //   this.log("newEl", el, el.outerHTML)
      // }, this)
      // forEachEls(oldEls, function(el) {
      //   this.log("oldEl", el, el.outerHTML)
      // }, this)
      throw "DOM doesn’t look the same on new loaded page: ’" + selector + "’ - new " + newEls.length + ", old " + oldEls.length
    }

    forEachEls(newEls, function(newEl, i) {
      var oldEl = oldEls[i]
      if (this.log) {
        this.log("newEl", newEl, "oldEl", oldEl)
      }

      if (switches[selector]) {
        switches[selector].bind(this)(oldEl, newEl, options, switchesOptions[selector])
      }
      else {
        defaultSwitches.outerHTML.bind(this)(oldEl, newEl, options)
      }
    }, this)
  }, this)
}

Alternatively, you could stick to one loop of selectors and build a queue of switch callbacks to execute after the loop is done and numPendingSwitches has been calculated.

See what you think of either of those approaches...

[robinnorth]

Maybe the queue approach is more elegant:

var forEachEls = require("./foreach-els")

var defaultSwitches = require("./switches")

module.exports = function(switches, switchesOptions, selectors, fromEl, toEl, options) {
  var queue = [];

  selectors.forEach(function(selector) {
    var newEls = fromEl.querySelectorAll(selector)
    var oldEls = toEl.querySelectorAll(selector)
    if (this.log) {
      this.log("Pjax switch", selector, newEls, oldEls)
    }
    if (newEls.length !== oldEls.length) {
      // forEachEls(newEls, function(el) {
      //   this.log("newEl", el, el.outerHTML)
      // }, this)
      // forEachEls(oldEls, function(el) {
      //   this.log("oldEl", el, el.outerHTML)
      // }, this)
      throw "DOM doesn’t look the same on new loaded page: ’" + selector + "’ - new " + newEls.length + ", old " + oldEls.length
    }

    this.state.numPendingSwitches += newEls.length

    forEachEls(newEls, function(newEl, i) {
      var oldEl = oldEls[i]
      if (this.log) {
        this.log("newEl", newEl, "oldEl", oldEl)
      }

      var callback = (switches[selector]) ?
        switches[selector].bind(this, oldEl, newEl, options, switchesOptions[selector]) :
        defaultSwitches.outerHTML.bind(this, oldEl, newEl, options)

      queue.push(callback)
    }, this)
  }, this)

  queue.forEach(function(callback) {
    callback()
  })
}

[BehindTheMath]

Even better, we don't need to increment this.state.numPendingSwitches manually. Instead, once all the switches are queued up, we can just do this.state.numPendingSwitches = queue.length.

[robinnorth]

Because this method refers to this.state a lot, you could do the following for brevity and readability:

var state = this.state;

window.history.pushState({
      url: state.href,
      title: state.options.title,
      uid: this.maxUid
},
state.options.title,
state.href)

This is just personal preference, but I find it makes things a bit easier to parse visually.

[BehindTheMath]

Good point.

[BehindTheMath]

@MoOx Can you add @robinnorth as a collaborator?

[tremby]

I'm not sure I can help. The PR description sounds valuable, but the changeset is too broad for my knowledge of the internals of this project. Bowing out.

[MoOx]

LGTM

[MoOx]

I would avoid mutation if possible and prefer a new object here.

[BehindTheMath]

Mutation of what?

[MoOx]

this.state. Imo it's better to avoid a mutation and create a new state (in case you want to compare a state change, it's easier to do state === prevState)
I am mostly doing react this days, and I was thinking about this https://reactjs.org/docs/react-component.html#shouldcomponentupdate :)

[BehindTheMath]

I'm still not sure what you mean. state is just a name I picked for the property to store the current href and options, so we can get it after the switches call back. After that, it gets destroyed (here). We could have used this.currentHref for the same results.

We could pass them around instead of making it a global property, but that gets messy and confusing.

[BehindTheMath]

Unless you mean we should do

this.state = {
    href: href,
    options: options
}

instead of

this.state.href = href
this.state.options = options

[MoOx]

That's what I mean. But you will probably need to copy the other state key/value.
Anyway, if you feel it's totally useless, feel free to move forward and ignore my comment. I am just quickly reviewing here as I currently don't use this library on any active project.

[BehindTheMath]

I don't see any reason to need the state once we're done with it, or to keep track of previous states, so I'm going to leave it for now.

[MoOx]

@BehindTheMath added!

[robinnorth]

Thanks @BehindTheMath, @MoOx!

I've added one small extra commit that hopefully takes care of your comment regarding state mutation, @MoOx.

Everything looks good to me, so I think you're good to merge, @BehindTheMath 👍

[BehindTheMath]

@tremby Thank you for your insightful comments in #72.

[BehindTheMath]

@robinnorth Thanks for all the comments and help.