Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
WebPush support for urgency and coalescing #213
WebPush support for urgency and coalescing #213
Changes from 10 commits
aeb4576
1bd283c
4add41b
9342c49
ab58c61
589e1dc
5ad1226
444537b
20203c0
2333379
cb63ad9
77bb5ff
8e9040a
cb171f6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
where does the deduplication actually take place, out of interest?
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.
We don't do this, it happens in the push gateway. If you have two messages with the same topic header, the second will replace the first (if the first hasn't been delivered yet).
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.
ah, so there is a separate "push gateway" after sygnal (which I tend to think of as a, y'know, push gateway)?
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 mean, webpush pipes notification to a push provider (or gateway, hard to name things correctly with so many parts to it) like FCM that will actually deliver it to the browser application, similarly to how the apns and gcm pushkins forward it to the push providers of those platforms. Every browser that supports web push needs to have a server like this that application servers can post push messages to. It's the browser-specific push provider that will drop the first message if it hasn't been deliver yet.
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.
this looks 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.
This still serves as something that looks like a response to pywebpush, while actually containing the information to perform the request ourselves once pywebpush finishes. I agree the naming is not ideal, but couldn't come up with something better. Ideas?
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.
right, I see.
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.
it now doesn't really happen "in the background", afaict? it's more that it is deferred until later.
How about:
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.
Indeed, I forgot to update this as the async request was fired immediately before but now is deferred to the execute method 👍 Have adopted your suggestion.