googlearchive / web-push-encryption

[Deprecated] Encryption Utilities for Web Push protocol
Apache License 2.0
84 stars 23 forks source link

Push 'succeeds' even when it doesn't #23

Closed petele closed 8 years ago

petele commented 8 years ago

If I try to send a push message to a client who has unsubscribed, the promise is fulfilled and I have to look at the response returned. I expected the promise to be rejected and then catch the error there.

webpush.sendWebPush(message, subscription, gcmKey)
.then(function(resp) {
  // May have sent or may not have
})
.catch(function(resp) {
  // Expected this to be called if the message wan't sent
});
gauntface commented 8 years ago

Where are two options here:

  1. We leave reject for unexpected errors (i.e. push server is down, network from your server is down) then use resolve for success, but we return an object with success and the network response
  2. We reject for all problem cases and pass in the network response above and set some kind of error code in the catch that highlights it's a bad subscription.

I'm more pro option 1 than 2 simply because we have controlled cases and we have unexpected / bad error cases.

@petele what do you think?

petele commented 8 years ago

How do other similar APIs that return promises behave?

While I don't love option 1, I can live with it if it's documented extremely well. The docs and samples should all include code snippets that check the success response for a true success.

gauntface commented 8 years ago

Spoke to Addy and he's pro option 2 like you so let's roll with that. It'll be demo code that is most important either way. Will look at a PR tomorrow for this hopefully.

gauntface commented 8 years ago

My current thinking for this would be this:

library.sendWebPush('Hello, World!', INVALID_SUBSCRIPTION)
.catch(err => {
  if (err.code === 'expired-subscription') {
      // TODO: Delete your subscription
      return;
  }
});

Does that seem good @addyosmani @petele ?

petele commented 8 years ago

Looks good to me! Thanks.

addyosmani commented 8 years ago

LGTM2

gauntface commented 8 years ago

It's in master - thanks all.