Use the same options object in handle-response as in send-request #148
Conversation
Instead of cloning this.options again in handle-response.js, pass the options object from send-request.js. This way, pjax.state.options will also have the request options.
lib/proto/attach-form.js
Outdated
| @@ -41,7 +41,7 @@ var formAction = function(el, event) { | |||
| // jscs:disable disallowImplicitTypeConversion | |||
| if (!!element.name && element.attributes !== undefined && tagName !== "button") { | |||
| var type = element.attributes.type | |||
| // jscs:enable disallowImplicitTypeConversion | |||
There was a problem hiding this comment.
Did you meant to remove this?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Additionally, once this is merged, how do you feel about pushing out another release? I think there's been some significant improvements and fixes since v0.2.5 that would be great to get out there. |
I agree. I think we should wait a few days to see if any other issues come up, and then we can push v0.2.6 and start working on v0.3.0/v1.0 |
|
@robinnorth Is there anything else that needs to be done here before merging? |
lib/proto/handle-response.js
Outdated
| tempOptions.request = request | ||
| module.exports = function(responseText, request, href, options) { | ||
| options = options || clone(this.options); | ||
| options.request = request |
There was a problem hiding this comment.
I agree it's useful to have the request available in this.state.options and in the options passed to the various Pjax events that are triggered, but perhaps the options object that is passed around should still be a clone to guard against accidental mutation of this.options by end users consuming the options object.
i.e.
options = options || clone(this.options)
options.request = request
var tempOptions = clone(this.options)
//...
this.state.options = tempOptionsThere was a problem hiding this comment.
I'm not sure which scenario you're trying to preempt.
Do you mean:
options = options ? clone(options) : clone(this.options)
options.request = request
//...
this.state.options = options
In case handleResponse() is called with this.options?
There was a problem hiding this comment.
What I'm trying to avoid is having this.options passed directly when events are triggered, as this PR changes trigger(document, "pjax:error", tempOptions) to trigger(document, "pjax:error", options), for example. Passing the options object directly could allow this.options to be mutated accidentally. The snippet you posted would prevent that from happening.
lib/proto/handle-response.js
Outdated
| var tempOptions = clone(this.options); | ||
| tempOptions.request = request | ||
| module.exports = function(responseText, request, href, options) { | ||
| options = options || clone(this.options); |
There was a problem hiding this comment.
Looks like an errant semicolon slipped through the net in a previous commit 😉
There was a problem hiding this comment.
I much prefer semicolons. I think it makes the code easier to read, and prevents any ASI errors. Unless there is objection, I would like to add them all back in at some point, so I'll just leave this here for now.
There was a problem hiding this comment.
I also much prefer them too, so that's fine by me.
lib/proto/attach-form.js
Outdated
| @@ -41,7 +41,7 @@ var formAction = function(el, event) { | |||
| // jscs:disable disallowImplicitTypeConversion | |||
| if (!!element.name && element.attributes !== undefined && tagName !== "button") { | |||
| var type = element.attributes.type | |||
| // jscs:enable disallowImplicitTypeConversion | |||
There was a problem hiding this comment.
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.
|
Sorry, I just realised that my code review was still 'pending'. Have published now so that you can see my comments. |
Instead of cloning
this.optionsagain inhandle-response.js, pass the options object fromsend-request.js. This way,pjax.state.optionswill also have the request options.