Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle XHR response error #137

Merged
merged 6 commits into from
Mar 15, 2018
Merged

Conversation

BehindTheMath
Copy link
Collaborator

  • Move the XHR callback to a separate file
  • Trigger an error event if the response cannot be parsed.

Fixes #123.

@BehindTheMath BehindTheMath requested a review from robinnorth March 9, 2018 15:21
module.exports = function(responseText, request, href) {
// Fail if unable to load HTML via AJAX
if (responseText === false) {
var tempOptions = this.options
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point.

this.loadContent(responseText, this.options)
}
catch (e) {
var tempOptions2 = this.options
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point.

@robinnorth
Copy link
Collaborator

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

@BehindTheMath
Copy link
Collaborator Author

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

@robinnorth
Copy link
Collaborator

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.

Copy link
Collaborator

@robinnorth robinnorth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@robinnorth robinnorth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@robinnorth robinnorth added this to the 0.2.6 milestone Mar 15, 2018
@BehindTheMath BehindTheMath merged commit 2166866 into master Mar 15, 2018
@BehindTheMath BehindTheMath deleted the feature/handle-response-error branch March 15, 2018 20:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants