Support Simple and Enhanced Notification Formats

[vanstee]

While looking into Issue #21 I realized there are 2 specific notification formats: Simple and Enhanced. It might be nice to also allow notifications that leave out the Identifier or Expiry in the payload (Simple format) since we only currently support the Enhanced version. I'd be ok with also carrying over the domain terms into new SimpleNotificiation and EnhancedNotification classes. Thoughts?

[kyledrake]

I think it would be nice to try to implement simple version, because it might help with #14. I'm not holding my breath, but it would be interesting to try!

[stevenharman]

As of tonight we now have a PassbookNotification so maybe that is precedent for splitting out SimpleNotification and EnhancedNotification objects?

[kyledrake]

That would be awesome! Supporting both is an awesome feature IMHO 👍

[kbrock]

@kyledrake funny you should say that this may help with #14 - Our previous C# (that only used the simple notification) code didn't seem to have an issue with the connection getting into a bad state and loosing notifications.

so there may be something to this.

In that case, you would just rely 100% on the feedback API.

Though listening to an active connection still has merits, especially when using the wrong key or something.

[kbrock]

Writing this now. any preference to whether this is a separate class, or if it is a notification with a switch on identifier being present or not?

[vanstee]

@kbrock: I think renaming the current Notification to EnhancedNotification and whipping up a new SimpleNotification is what I had in mind. @stevenharman and @alindeman Thoughts?

[stevenharman]

After learning more about Simple Notifications, I have to question... why do we want to support them? In reality they are just the original implementation, and the new "enhanced" format is better anyhow.

[kbrock]

The question was:
Will we have the same issues after pushing a bad notification in a version 0 message format?

I hacked together #47 to try it out. But in the end of the day, I think we have the same issues and we don't have the identifier to resolve issues.
Also #47 will probably break existing clients, without much gain.

So either:

a) implement Enhanced in Notification and have Simple / V0 as the alternative format
b) don't support v0 notifications at all

Again, I was coding up the example anyway, and since I saw it elsewhere, thought I'd share.

[stevenharman]

@kbrock Thanks so much for putting this together and sharing it. I was replying from my iPhone and so quite terse... which probably came off as if I were unappreciative.

I agree with the two options you've outlined, and personally think we should go for B - don't support version 0 notifications. We don't really gain anything by supporting them, and they are harder to handle w/r/t errors.

What say we all?

[kbrock]

I closed #47 - someone please kill this request

[kbrock]

sorry @vanstee ;)

[stevenharman]

Based on the discussion here and in #47, we're going to opt not to support Simple Notifications, at least for now.