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

Improve error handling #38

Merged
merged 4 commits into from
Apr 23, 2018
Merged

Improve error handling #38

merged 4 commits into from
Apr 23, 2018

Conversation

dhruvdutt
Copy link

Fixes #36 #163

@divyanshu013
Copy link

Why is the fetch request not getting caught in case of status >= 400?

@dhruvdutt
Copy link
Author

dhruvdutt commented Apr 20, 2018

@divyanshu013 I think that's how it works. An error is not an exception. 🤔 😄

I think some fetch libraries might have different implementation though.

Further reading: #36 #18, #203.

@dhruvdutt dhruvdutt requested a review from divyanshu013 April 20, 2018 07:30
package.json Outdated
@@ -1,6 +1,6 @@
{
"name": "appbase-js",
"version": "2.3.0",
"version": "2.3.1",
Copy link
Author

Choose a reason for hiding this comment

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

Would this be a patch release or something else?

I hope it doesn't break things for those who depends on status 400 inside .on('data').

cc @divyanshu013 😋

Choose a reason for hiding this comment

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

Please do a major release 🙏

Copy link

@divyanshu013 divyanshu013 left a comment

Choose a reason for hiding this comment

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

Looks good, if you can add tests for these cases that would be awesome 🔥

Copy link

@metagrover metagrover left a comment

Choose a reason for hiding this comment

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

LGTM!

@metagrover metagrover merged commit c9ecb36 into appbaseio:master Apr 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants