Add support for parsing json response#32
Add support for parsing json response#32JakeDluhy wants to merge 3 commits intopretenderjs:masterfrom
Conversation
|
I just rebased on top of master. Any chance I could get someone to look at this? |
|
Bump. Having same issue here, would love to see this PR merged 👍 |
fake_xml_http_request.js
Outdated
| try { | ||
| this.response = JSON.parse(this.responseText); | ||
| } catch (e) { | ||
| // Unable to parse JSON - no biggie |
There was a problem hiding this comment.
lets only catch errors related to parse failure. That we we don't swallow unexpected errors
src/fake-xml-http-request.js
Outdated
| try { | ||
| this.response = JSON.parse(this.responseText); | ||
| } catch (e) { | ||
| // Unable to parse JSON - no biggie |
fake_xml_http_request.js
Outdated
| } catch (e) { | ||
| // Unable to parse XML - no biggie | ||
| } | ||
| } else if(this.responseText && /(application\/json)|(application\/vnd\.api\+json)/.test(type)) { |
There was a problem hiding this comment.
space between if and (
src/fake-xml-http-request.js
Outdated
| } catch (e) { | ||
| // Unable to parse XML - no biggie | ||
| } | ||
| } else if(this.responseText && /(application\/json)|(application\/vnd\.api\+json)/.test(type)) { |
There was a problem hiding this comment.
space between if and (
|
left some things that need to be addressed. I believe this is a good change, and correct me if I'm wrong it will require a major version bump, right? |
|
I made the requested changes. I don't think it will require a major version bump. It only adds the I also added another commit updating the README to have the extra key in the documentation. |
|
I think this is related to my issue over at miragejs/ember-cli-mirage#1320. Any chance this can be merged? What else is left to do? |
|
@stefanpenner I believe this is good – ok to merge? |
| } | ||
| } else if (this.responseText && /(application\/json)|(application\/vnd\.api\+json)/.test(type)) { | ||
| try { | ||
| this.response = JSON.parse(this.responseText); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
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
|
@stefanpenner what do you think of the proposed changes? |
|
See #32 (comment). These changes don’t follow the XHR spec |
|
@samselikoff I think @nlfurniss's concerns likely need to be addressed. Spec violations can lead to some serious compat issues... |
6 participants
Fixes #31