-
Notifications
You must be signed in to change notification settings - Fork 43
Move to using just endpoint #129
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
Conversation
Oh yeah, I forgot, this doesn't update the big sequence diagram. I might get to that, but without the original source, I'd have to start over. |
I agree that we need to make a decision about this, while I'm still erring towards the separated properties side myself. The problem is that no one feels strongly about it - there's arguments both for keeping it and for removing it. Let's wait for @mvano's return and then find a way to drive this to a solution. |
index.html
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you lost some meaningful information in this heading. How about something like "Push service use" as what we really care about here is pointing to the IETF Web Push Protocol.
Thanks for the simplifications Martin! Now that the IETF work is progressing and has support from Mozilla, Microsoft, Google, and interest from others as well, this generally looks good to me. If you could address some of my comments, this pull request should get merged ASAP. |
index.html
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added the sequence diagram source in vdx format. I made it in Lucidchart, and editing it should work well there.
Note TODO on diagram Rework old and broken section
c70d91b
to
1869819
Compare
OK, I just had a shot at this. I made some fairly big changes to the diagram, so check those over. I don't know how you want to do comments; maybe I can share it in Lucidchart. |
I think it is way better to understand. The only minor things I found: Shouldn't clear push subscription happen after unsubscription, so that you can wait for it to resolve successfully before you delete it? |
@paul-em, you shouldn't remove a subscription before you tell everyone that is using it to stop using it. Otherwise, they will expect to have messages delivered to you and (probably) get errors. set vs save vs store matters not to me (other than the time it will now take to regenerate all the files). I thought that set and clear were neatly symmetrical, but didn't really think that hard about it. |
Oh ok makes sense to remove before then. |
Martin, the diagram changes generally look good to me. Just a few nits:
|
Hmm, maybe you can address those nits later, let's just get this merged for now. |
Use just endpoint, drop subscription id, diagram cleanup, refer to draft IETF web push protocol
There was a lot of text here that was simply wrong, probably because it was outdated.
I've removed or reworked that part. I haven't cited the IETF work (yet), but there is a placeholder there.
Closes #56.