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

Handle XHR response error #137

Merged
BehindTheMath merged 6 commits from feature/handle-response-error into master 2018-03-15 15:12:32 -05:00
Conversation 9 Commits 6 Files Changed 5
+310 -65
BehindTheMath commented 2018-03-09 10:21:41 -05:00 (Migrated from github.com)
Copy link
Copy source
  • Move the XHR callback to a separate file
  • Trigger an error event if the response cannot be parsed.

Fixes #123.

* Move the XHR callback to a separate file * Trigger an error event if the response cannot be parsed. Fixes #123.
robinnorth (Migrated from github.com) reviewed 2018-03-10 13:01:00 -05:00
lib/proto/handle-response.js
robinnorth (Migrated from github.com) commented 2018-03-10 13:01:00 -05:00
Copy link
Copy source

This needs to be tempOptions = clone(this.options), or you'll mutate the current instance's options

This needs to be `tempOptions = clone(this.options)`, or you'll mutate the current instance's options
robinnorth (Migrated from github.com) reviewed 2018-03-10 13:02:53 -05:00
lib/proto/handle-response.js
robinnorth (Migrated from github.com) commented 2018-03-10 13:02:53 -05:00
Copy link
Copy source

This also needs to clone this.options, but you could probably just do this once at the top of the method rather than repeating the assignment of the request to your temporary event options.

This also needs to clone `this.options`, but you could probably just do this once at the top of the method rather than repeating the assignment of the request to your temporary event options.
BehindTheMath (Migrated from github.com) reviewed 2018-03-10 23:18:18 -05:00
lib/proto/handle-response.js
BehindTheMath (Migrated from github.com) commented 2018-03-10 23:18:18 -05:00
Copy link
Copy source

Good point.

Good point.
BehindTheMath (Migrated from github.com) reviewed 2018-03-10 23:21:19 -05:00
lib/proto/handle-response.js
BehindTheMath (Migrated from github.com) commented 2018-03-10 23:21:19 -05:00
Copy link
Copy source

Good point.

Good point.
robinnorth commented 2018-03-12 08:17:21 -05:00 (Migrated from github.com)
Copy link
Copy source

It's all looking good to me now, do you want to add some tests for handle-response?

It's all looking good to me now, do you want to add some tests for `handle-response`?
BehindTheMath commented 2018-03-12 08:28:06 -05:00 (Migrated from github.com)
Copy link
Copy source

Yes, please. You're better at that than I am.

Yes, please. You're better at that than I am.
robinnorth commented 2018-03-12 08:46:04 -05:00 (Migrated from github.com)
Copy link
Copy source

I don't have much time to do that at present, unfortunately, so you can either have a go yourself or, as handle-response is largely a direct transplant from index.js and as most of the remainder of that file is untested as well, we could park adding tests for this until #21 if you want to get this merged soon.

I don't have much time to do that at present, unfortunately, so you can either have a go yourself or, as `handle-response` is largely a direct transplant from `index.js` and as most of the remainder of that file is untested as well, we could park adding tests for this until #21 if you want to get this merged soon.
robinnorth (Migrated from github.com) reviewed 2018-03-14 15:20:09 -05:00
robinnorth (Migrated from github.com) left a comment
Copy link
Copy source

Thanks for adding tests, I'll try and find some time to review them shortly.

Thanks for adding tests, I'll try and find some time to review them shortly.
robinnorth (Migrated from github.com) approved these changes 2018-03-15 04:36:23 -05:00
robinnorth (Migrated from github.com) left a comment
Copy link
Copy source

Took a look at the tests this morning, all seems good to merge to me!

Took a look at the tests this morning, all seems good to merge to me!
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 labels
Milestone
No items
No Milestone 0.2.6
Projects
Clear projects
No projects
Assignees
Clear assignees
iLoveElysia
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: iLoveElysia/pjax#137