Use the same options object in handle-response as in send-request #148

Merged
BehindTheMath merged 3 commits from fix/options-and-state into master 2018-04-26 08:27:51 -05:00
5 changed files with 20 additions and 14 deletions

3
index.d.ts vendored
View File

@@ -67,8 +67,9 @@ declare class Pjax {
* @param {string} requestText The raw text of the response. Same as <code>request.responseText</code>. * @param {string} requestText The raw text of the response. Same as <code>request.responseText</code>.
* @param {XMLHttpRequest} request The XHR object. * @param {XMLHttpRequest} request The XHR object.
* @param {string} href The original URI used to initiate the request. * @param {string} href The original URI used to initiate the request.
* @param options The Pjax options object used for the request
*/ */
handleResponse(requestText: string, request: XMLHttpRequest, href: string): void; handleResponse(requestText: string, request: XMLHttpRequest, href: string, options?: Pjax.IOptions): void;
/** /**
* Initiates the request by calling <code>doRequest()</code>. * Initiates the request by calling <code>doRequest()</code>.

View File

@@ -40,8 +40,9 @@ var formAction = function(el, event) {
var tagName = element.tagName.toLowerCase() var tagName = element.tagName.toLowerCase()
robinnorth commented 2018-04-13 03:55:49 -05:00 (Migrated from github.com)
Review

Did you meant to remove this?

Did you meant to remove this?
BehindTheMath commented 2018-04-13 08:39:45 -05:00 (Migrated from github.com)
Review

Yes. The tests run fine without it, so it's not necessary.

I should have added it to #147, but I forgot to do it before I merged it.

Yes. The tests run fine without it, so it's not necessary. I should have added it to #147, but I forgot to do it before I merged it.
robinnorth commented 2018-04-13 09:48:33 -05:00 (Migrated from github.com)
Review

If you're explicitly disabling a test for a given code block with an inline comment, I think you should always re-enable it after to ensure that you don't accidentally lose coverage... or, alternatively, just disable the rule globally if you don't find it to be of any use.

In this case, // jscs:disable disallowImplicitTypeConversion should be paired with a // jscs:enable disallowImplicitTypeConversion somewhere or we should disable the rule altogether, IMO. Personally, I'd be quite happy to disable it, as I find !!element.name to be quite a convenient shorthand for casting to a Boolean.

If you're explicitly disabling a test for a given code block with an inline comment, I think you should always re-enable it after to ensure that you don't accidentally lose coverage... or, alternatively, just disable the rule globally if you don't find it to be of any use. In this case, `// jscs:disable disallowImplicitTypeConversion` should be paired with a `// jscs:enable disallowImplicitTypeConversion` somewhere or we should disable the rule altogether, IMO. Personally, I'd be quite happy to disable it, as I find `!!element.name` to be quite a convenient shorthand for casting to a Boolean.
BehindTheMath commented 2018-04-22 17:23:09 -05:00 (Migrated from github.com)
Review

My bad. I just glanced quickly at it and I thought they were both disable rules, applicable to only the next line.

I'll revert that. Thanks for catching that.

My bad. I just glanced quickly at it and I thought they were both `disable` rules, applicable to only the next line. I'll revert that. Thanks for catching that.
// jscs:disable disallowImplicitTypeConversion // jscs:disable disallowImplicitTypeConversion
if (!!element.name && element.attributes !== undefined && tagName !== "button") { if (!!element.name && element.attributes !== undefined && tagName !== "button") {
var type = element.attributes.type
// jscs:enable disallowImplicitTypeConversion // jscs:enable disallowImplicitTypeConversion
var type = element.attributes.type
if ((!type || type.value !== "checkbox" && type.value !== "radio") || element.checked) { if ((!type || type.value !== "checkbox" && type.value !== "radio") || element.checked) {
// Build array of values to submit // Build array of values to submit
var values = [] var values = []

View File

@@ -2,13 +2,13 @@ var clone = require("../util/clone.js")
var newUid = require("../uniqueid.js") var newUid = require("../uniqueid.js")
var trigger = require("../events/trigger.js") var trigger = require("../events/trigger.js")
module.exports = function(responseText, request, href) { module.exports = function(responseText, request, href, options) {
var tempOptions = clone(this.options); options = clone(options || this.options)
tempOptions.request = request options.request = request
// Fail if unable to load HTML via AJAX // Fail if unable to load HTML via AJAX
if (responseText === false) { if (responseText === false) {
trigger(document, "pjax:complete pjax:error", tempOptions) trigger(document, "pjax:complete pjax:error", options)
return return
} }
@@ -49,13 +49,13 @@ module.exports = function(responseText, request, href) {
} }
this.state.href = href this.state.href = href
this.state.options = clone(this.options) this.state.options = options
try { try {
this.loadContent(responseText, this.options) this.loadContent(responseText, this.options)
} }
catch (e) { catch (e) {
trigger(document, "pjax:error", tempOptions) trigger(document, "pjax:error", options)
if (!this.options.debug) { if (!this.options.debug) {
if (console && console.error) { if (console && console.error) {

View File

@@ -13,21 +13,21 @@ module.exports = function(location, options, callback) {
request.onreadystatechange = function() { request.onreadystatechange = function() {
if (request.readyState === 4) { if (request.readyState === 4) {
if (request.status === 200) { if (request.status === 200) {
callback(request.responseText, request, location) callback(request.responseText, request, location, options)
} }
else if (request.status !== 0) { else if (request.status !== 0) {
callback(null, request, location) callback(null, request, location, options)
} }
} }
} }
request.onerror = function(e) { request.onerror = function(e) {
console.log(e) console.log(e)
callback(null, request, location) callback(null, request, location, options)
} }
request.ontimeout = function() { request.ontimeout = function() {
callback(null, request, location) callback(null, request, location, options)
} }
// Prepare the request payload for forms, if available // Prepare the request payload for forms, if available

View File

@@ -37,7 +37,9 @@ tape("test events triggered when handleResponse(false) is called", function(t) {
tape("test when handleResponse() is called normally", function(t) { tape("test when handleResponse() is called normally", function(t) {
var pjax = { var pjax = {
options: {}, options: {
test: 1
},
loadContent: noop, loadContent: noop,
state: {} state: {}
} }
@@ -55,7 +57,9 @@ tape("test when handleResponse() is called normally", function(t) {
scrollPos: [0, 0] scrollPos: [0, 0]
}, "window.history.state is set correctly") }, "window.history.state is set correctly")
t.equals(pjax.state.href, "href", "this.state.href is set correctly") t.equals(pjax.state.href, "href", "this.state.href is set correctly")
t.same(pjax.state.options, pjax.options, "this.state.options is set correctly") t.equals(Object.keys(pjax.state.options).length, 2, "this.state.options is set correctly")
t.same(pjax.state.options.request, request, "this.state.options is set correctly")
t.equals(pjax.state.options.test, 1, "this.state.options is set correctly")
t.end() t.end()
}) })