Handle XHR response error #137

2018-03-09T10:21:41-05:00

BehindTheMath commented

2018-03-09 10:21:41 -05:00

(Migrated from github.com)

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

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

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

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

Good point.

robinnorth commented

2018-03-12 08:17:21 -05:00

(Migrated from github.com)

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)

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

robinnorth commented

2018-03-12 08:46:04 -05:00

(Migrated from github.com)

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

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

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

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: iLoveElysia/pjax#137