Fix async switches #110

2018-01-21T00:47:33-05:00

BehindTheMath commented

2018-01-21 00:47:33 -05:00

(Migrated from github.com)

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.

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 commented

2018-01-21 01:01:49 -05:00

(Migrated from github.com)

@tremby What do you think?

tremby commented

2018-01-21 02:31:54 -05:00

(Migrated from github.com)

Will read and respond tomorrow.

robinnorth (Migrated from github.com) requested changes

2018-01-21 07:36:40 -05:00

robinnorth (Migrated from github.com) left a comment

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!

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!

lib/switches-selectors.js

robinnorth (Migrated from github.com) commented

2018-01-21 07:30:26 -05:00

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.

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: ```js 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.

robinnorth (Migrated from github.com) reviewed

2018-01-21 07:46:28 -05:00

index.js

robinnorth (Migrated from github.com) commented

2018-01-21 07:46:28 -05:00

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.

Because this method refers to `this.state` a lot, you could do the following for brevity and readability: ```js 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 (Migrated from github.com) reviewed

2018-01-21 09:33:13 -05:00

index.js

BehindTheMath (Migrated from github.com) commented

2018-01-21 09:33:12 -05:00

Good point.

BehindTheMath (Migrated from github.com) reviewed

2018-01-21 09:33:25 -05:00

lib/switches-selectors.js

BehindTheMath (Migrated from github.com) commented

2018-01-21 09:33:25 -05:00

Good point.

BehindTheMath (Migrated from github.com) reviewed

2018-01-21 09:42:00 -05:00

lib/switches-selectors.js

BehindTheMath (Migrated from github.com) commented

2018-01-21 09:42:00 -05:00

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

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

robinnorth (Migrated from github.com) reviewed

2018-01-21 10:30:53 -05:00

lib/switches-selectors.js

robinnorth (Migrated from github.com) commented

2018-01-21 10:30:53 -05:00

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...

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): ```js 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 (Migrated from github.com) reviewed

2018-01-21 10:39:00 -05:00

lib/switches-selectors.js

robinnorth (Migrated from github.com) commented

2018-01-21 10:39:00 -05:00

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()
  })
}

Maybe the queue approach is more elegant: ```js 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 (Migrated from github.com) reviewed

2018-01-21 14:13:14 -05:00

lib/switches-selectors.js

BehindTheMath (Migrated from github.com) commented

2018-01-21 14:13:14 -05:00

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.

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`.

BehindTheMath commented

2018-01-21 15:41:00 -05:00

(Migrated from github.com)

@MoOx Can you add @robinnorth as a collaborator?

tremby commented

2018-01-22 02:22:07 -05:00

(Migrated from github.com)

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 (Migrated from github.com) approved these changes

2018-01-22 08:08:18 -05:00

MoOx (Migrated from github.com) left a comment

LGTM

index.js

MoOx (Migrated from github.com) commented

2018-01-22 08:07:10 -05:00

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

MoOx commented

2018-01-22 08:08:30 -05:00

(Migrated from github.com)

@BehindTheMath added!

🎉 2

robinnorth commented

2018-01-22 09:56:16 -05:00

(Migrated from github.com)

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 👍

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 (Migrated from github.com) reviewed

2018-01-22 10:15:04 -05:00

index.js

BehindTheMath (Migrated from github.com) commented

2018-01-22 10:15:04 -05:00

Mutation of what?

MoOx (Migrated from github.com) reviewed

2018-01-22 10:17:13 -05:00

index.js

MoOx (Migrated from github.com) commented

2018-01-22 10:17:13 -05:00

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 :)

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 (Migrated from github.com) reviewed

2018-01-22 10:24:26 -05:00

index.js

BehindTheMath (Migrated from github.com) commented

2018-01-22 10:24:25 -05:00

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.

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](https://github.com/MoOx/pjax/pull/110/files#diff-168726dbe96b3ce427e7fedce31bb0bcR263)). 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 commented

2018-01-22 10:27:46 -05:00

(Migrated from github.com)

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

BehindTheMath (Migrated from github.com) reviewed

2018-01-22 10:42:00 -05:00

index.js

BehindTheMath (Migrated from github.com) commented

2018-01-22 10:42:00 -05:00

Unless you mean we should do

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

instead of

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

Unless you mean we should do this.state = { href: href, options: options } instead of this.state.href = href this.state.options = options

MoOx (Migrated from github.com) reviewed

2018-01-22 10:49:17 -05:00

index.js

MoOx (Migrated from github.com) commented

2018-01-22 10:49:17 -05:00

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.

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.