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

feat: improve response error codes #39

Merged
merged 3 commits into from
Jul 1, 2017

Conversation

glennr
Copy link
Contributor

@glennr glennr commented Jun 16, 2017

  1. This library only currently supports specific 400/410 errors with InvalidSubscription
  2. For all other errors its difficult to determine what type of error occurred as ResponseError only provides a stringified version of the Net/HTTPResponse.

However the Web Push Protocol defines several possible responses;

https://developers.google.com/web/fundamentals/engage-and-retain/push-notifications/web-push-protocol#response_from_push_service

This PR adds the following first-class error types;

  • ExpiredSubscription (404)
  • PayloadTooLarge (413)
  • TooManyRequests (429)

It also adds the original Net/HTTPResponse error as an attribute on ResponseError.

@glennr
Copy link
Contributor Author

glennr commented Jun 16, 2017

Just realized I only had spec coverage in the GCM section - whereas I thought I was adding it under VAPID - which is the protocol linked above.

I haven't tested the GCM stuff, but even if the GCM protocol doesn't work the same, each of these more specific error codes is still a ResponseError under the hood - so any existing code relying on that error class shouldn't break.

@zaru
Copy link
Owner

zaru commented Jul 1, 2017

@glennr Thanks for PR! Great😀

@zaru zaru merged commit 90a352a into zaru:master Jul 1, 2017
@zaru
Copy link
Owner

zaru commented Jul 1, 2017

I released v0.3.2
https://rubygems.org/gems/webpush/versions/0.3.2

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.

2 participants