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

Vanilla: Call onError without argument if there is no response #9

Merged
merged 1 commit into from
Feb 21, 2017
Merged

Conversation

minad
Copy link
Contributor

@minad minad commented Jan 28, 2017

@minad
Copy link
Contributor Author

minad commented Feb 9, 2017

ping @soenkehahn

@phadej
Copy link
Contributor

phadej commented Feb 9, 2017

  • it would be good to check that Content-Type isapplication/json
  • Also it would be great to JSON.parse inside try / catch block

@minad
Copy link
Contributor Author

minad commented Feb 9, 2017

Ok, I am pretty sure this can be improved. My reasoning is that if JSON is served, it should be valid - this PR only treats the case of no response.

But I am still not sure about what is desired here:

  1. Should onError called without an argument or with some kind of error object? If we want an error object, which format, e.g. { status: ..., msg: ... }?
  2. Should onError be called if JSON parsing fails or should there be an exception?

@phadej
Copy link
Contributor

phadej commented Feb 9, 2017

My concerns are that in not 2xx case the response might be (and in case of say 500 exception, isn't) a valid JSON. So maybe I'd just onError(new Error(xhr))

@minad
Copy link
Contributor Author

minad commented Feb 10, 2017

I updated the patch

<> " var value = JSON.parse(xhr.responseText);\n"
<> " onError(value);\n"
<> " try { res = JSON.parse(xhr.responseText); } catch (e) { " <> onError <> "(e); }\n"
<> " if (res) " <> onSuccess <> "(res);\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

onSuccess, it should be onError?

@minad
Copy link
Contributor Author

minad commented Feb 11, 2017

ouch yes

Copy link
Contributor

@phadej phadej left a comment

Choose a reason for hiding this comment

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

LGTM, I'll try it and merge then. Thanks.

@phadej phadej merged commit 21e7a8a into haskell-servant:master Feb 21, 2017
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.

None yet

2 participants