iLoveElysia/pjax
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 9 Wiki Activity

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
Conversation 13 Commits 3 Files Changed 5 +20 -14
BehindTheMath commented 2018-04-12 17:34:51 -05:00 (Migrated from github.com)
Copy Link

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.

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.
robinnorth (Migrated from github.com) reviewed 2018-04-13 03:55:49 -05:00
lib/proto/attach-form.js
@@ -40,8 +40,9 @@ var formAction = function(el, event) {
var tagName = element.tagName.toLowerCase()
robinnorth (Migrated from github.com) commented 2018-04-13 03:55:49 -05:00
Copy Link

Did you meant to remove this?

Did you meant to remove this?
robinnorth commented 2018-04-13 04:15:41 -05:00 (Migrated from github.com)
Copy Link

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.

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.
BehindTheMath (Migrated from github.com) reviewed 2018-04-13 08:39:45 -05:00
lib/proto/attach-form.js
@@ -40,8 +40,9 @@ var formAction = function(el, event) {
var tagName = element.tagName.toLowerCase()
BehindTheMath (Migrated from github.com) commented 2018-04-13 08:39:45 -05:00
Copy Link

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.
BehindTheMath commented 2018-04-13 08:41:39 -05:00 (Migrated from github.com)
Copy Link

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

>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
👍 1
BehindTheMath commented 2018-04-19 23:02:27 -05:00 (Migrated from github.com)
Copy Link

@robinnorth Is there anything else that needs to be done here before merging?

@robinnorth Is there anything else that needs to be done here before merging?
robinnorth (Migrated from github.com) reviewed 2018-04-20 04:37:28 -05:00
lib/proto/attach-form.js
@@ -40,8 +40,9 @@ var formAction = function(el, event) {
var tagName = element.tagName.toLowerCase()
robinnorth (Migrated from github.com) commented 2018-04-13 09:48:33 -05:00
Copy Link

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.
lib/proto/handle-response.js
robinnorth (Migrated from github.com) commented 2018-04-13 04:00:06 -05:00
Copy Link

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 = tempOptions
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. ```js options = options || clone(this.options) options.request = request var tempOptions = clone(this.options) //... this.state.options = tempOptions ```
robinnorth (Migrated from github.com) commented 2018-04-13 04:13:37 -05:00
Copy Link

Looks like an errant semicolon slipped through the net in a previous commit 😉

Looks like an errant semicolon slipped through the net in a previous commit 😉
robinnorth commented 2018-04-20 04:38:00 -05:00 (Migrated from github.com)
Copy Link

Sorry, I just realised that my code review was still 'pending'. Have published now so that you can see my comments.

Sorry, I just realised that my code review was still 'pending'. Have published now so that you can see my comments.
BehindTheMath (Migrated from github.com) reviewed 2018-04-22 17:23:09 -05:00
lib/proto/attach-form.js
@@ -40,8 +40,9 @@ var formAction = function(el, event) {
var tagName = element.tagName.toLowerCase()
BehindTheMath (Migrated from github.com) commented 2018-04-22 17:23:09 -05:00
Copy Link

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.
BehindTheMath (Migrated from github.com) reviewed 2018-04-22 17:28:43 -05:00
lib/proto/handle-response.js
BehindTheMath (Migrated from github.com) commented 2018-04-22 17:28:42 -05:00
Copy Link

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.

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.
BehindTheMath (Migrated from github.com) reviewed 2018-04-22 17:34:35 -05:00
lib/proto/handle-response.js
BehindTheMath (Migrated from github.com) commented 2018-04-22 17:34:35 -05:00
Copy Link

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?

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`?
robinnorth (Migrated from github.com) reviewed 2018-04-23 04:14:25 -05:00
lib/proto/handle-response.js
robinnorth (Migrated from github.com) commented 2018-04-23 04:14:25 -05:00
Copy Link

I also much prefer them too, so that's fine by me.

I also much prefer them too, so that's fine by me.
robinnorth (Migrated from github.com) reviewed 2018-04-24 04:12:53 -05:00
lib/proto/handle-response.js
robinnorth (Migrated from github.com) commented 2018-04-24 04:12:53 -05:00
Copy Link

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.

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.
robinnorth (Migrated from github.com) approved these changes 2018-04-26 03:10:37 -05:00
Sign in to join this conversation.
Reviewers
No Reviewers
BehindTheMath
robinnorth
Labels
Clear labels
bug
dependencies
Pull requests that update a dependency file
 duplicate
enhancement
invalid
question
wontfix
No Label
Milestone
No items
No Milestone 0.2.6
Projects
Clear projects
No project
Assignees
Clear assignees
iLoveElysia
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: iLoveElysia/pjax#148
Delete Branch "fix/options-and-state"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?