Add safeStreamJSON function #10
Conversation
BrendanDavies
force-pushed
the
streamIssue
branch
2 times, most recently
from
46a552b
to
982f2c0
Compare
|
I still need to write tests for this, but wanted to float the solution by people first. |
src/hyperGard.js
Outdated
| } : Promise.reject(); | ||
| }) | ||
| .catch(function() { | ||
| return Promise.reject({ |
There was a problem hiding this comment.
It might be helpful to include the underlying error here for easier debugging when homepage loads fail
There was a problem hiding this comment.
Good call, will add that.
I actually don't see a lot of value in having separate onError/onSuccess handlers for the homepage.
I might just consolidate, so we get more consistent results
src/hyperGard.js
Outdated
| return isObject(data) ? { | ||
| data: new Resource(endpoint, data), | ||
| xhr: response | ||
| } : Promise.reject(); |
There was a problem hiding this comment.
I may be missing something, but it looks like we could add some context to this rejection
There was a problem hiding this comment.
Yea absolutely, I was just addressing the streaming issue in this patch.
I can adjust that as well
BrendanDavies
force-pushed
the
streamIssue
branch
from
982f2c0
to
c6cc22b
Compare
|
Addressed comments, but I still need tests for safeStreamJSON library |
src/hyperGard.js
Outdated
| var o = deepExtend({ | ||
| method: 'GET', | ||
| action: 'homepage', | ||
| action: action, |
There was a problem hiding this comment.
Elsewhere in this file, the variable name is used to represent an action name and the variable action is used to represent an action object. Mind giving it a bit of a rename? It would be helpful to people who may be diving into this for the first time.
| */ | ||
| export function safeStreamJSON(response) { | ||
| if (response && response instanceof Response && !response.bodyUsed) { | ||
| return response.text() |
There was a problem hiding this comment.
Curious why you went with response.text & JSON.parse instead of response.json?
There was a problem hiding this comment.
Response.json calls text, and the json.parse, but if the streamed text is NOT valid json, you lose that text.
var response = new Response('Thing'); response.json()
Will result in an error Uncaught (in promise) SyntaxError: Unexpected token T in JSON at position 0 at <anonymous>:2:10
But since the body has already been used, and then text was not returned in the error message, we don't have any more context to that failure.
#8 Add safeStremJSON Function
What:
Add helper function to stream/parse json responses.
Why:
This allows us to comfortably stream and parse a response, without fear of loosing valuable response information, if the stream is already ready OR not valid JSON