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.

Sign up for GitHub

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

Add support for parsing json response #32

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add support for parsing json response #32

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
848e755
Add support for parsing json response
JakeDluhy Apr 4, 2017
adf6e15
Make CR changes: change if spacing and error catching
JakeDluhy Mar 2, 2018
f43cdf4
Update README detailing the additional response key
JakeDluhy Mar 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions fake_xml_http_request.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -447,6 +447,12 @@
} catch (e) {
// Unable to parse XML - no biggie
}
} else if(this.responseText && /(application\/json)|(application\/vnd\.api\+json)/.test(type)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

space between if and (

try {
this.response = JSON.parse(this.responseText);
Copy link

Choose a reason for hiding this comment

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

This isn't correct in terms of the spec. There is always a response no matter the responseType, but there is only a responseText when response type is text. response should be whatever the response type called for: string for text, xml for xml, object for json, etc.

https://xhr.spec.whatwg.org/#the-response-attribute

Copy link
Contributor

Choose a reason for hiding this comment

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

@nlfurniss thanks, yes we should be careful to not deviate from the spec.

Could you briefly describe the implications of this violation? I suspect you have a use-case in-mind, and rather then guessing what the use-case is, knowing it will help the rest of us think about this correctly.

I likely have some follow up questions.

Also, do you see a path forward for the PR, or does your concern invalidate it entirely?

Copy link

Choose a reason for hiding this comment

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

My use-case is JSON, but it seemed like this semi-completeness might confuse people.

After looking at it again, I don't think it's a blocker but I'll create an issue after it lands setting out that response is not 100% supported

} catch (e) {
// Unable to parse JSON - no biggie
Copy link
Contributor

Choose a reason for hiding this comment

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

lets only catch errors related to parse failure. That we we don't swallow unexpected errors

}
}

if (this.async) {
Expand Down
6 changes: 6 additions & 0 deletions src/fake-xml-http-request.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -441,6 +441,12 @@ var FakeXMLHttpRequestProto = {
} catch (e) {
// Unable to parse XML - no biggie
}
} else if(this.responseText && /(application\/json)|(application\/vnd\.api\+json)/.test(type)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

space between if and (

try {
this.response = JSON.parse(this.responseText);
} catch (e) {
// Unable to parse JSON - no biggie
Copy link
Contributor

Choose a reason for hiding this comment

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

^

}
}

if (this.async) {
Expand Down
11 changes: 11 additions & 0 deletions test/responding_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,17 @@ test("does not parse the body if it's XML and another content type is set", func
equal(xhr.responseXML, undefined);
});

test("parses the body if it's JSON and the json content type is set", function(){
const body = { key: 'value' };
xhr.respond(200, {'Content-Type':'application/json'}, JSON.stringify(body));
deepEqual(xhr.response, body);
});

test("does not parse the JSON body when another content type is set", function() {
xhr.respond(200, {'Content-Type':'application/xml'}, '{"a":"key"}');
equal(xhr.response, undefined);
});

test("calls the onload callback once", function(){
var wasCalled = 0;

Expand Down