-
Notifications
You must be signed in to change notification settings - Fork 146
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
Add experimental support for WebPush #177
Conversation
A service worker can only have 1 push subscription, but for web clients that handle multiple matrix accounts like hydrogen, we want to be able to enable push for every account. We can do that by setting a pusher with the same web push details for each account, but we can't really tell which account a push notification is for, so forward a session_id set on the pusher data.
Tried to address as many outstanding issues as I could. A few things to note:
|
57e26d5
to
35e0a0e
Compare
Co-authored-by: Patrick Cloke <clokep@users.noreply.github.com>
@bwindels Can you double check over the changes I pushed and make sure they seem reasonable? If so I think this is ✅ ! |
Thank you for your changes @clokep! Lgtm (one minor comment). |
sygnal/webpushpushkin.py
Outdated
@@ -102,7 +102,7 @@ def __init__(self, name, sygnal, config): | |||
raise PushkinSetupException("path in 'vapid_private_key' does not exist") | |||
try: | |||
self.vapid_private_key = Vapid.from_file(private_key_file=privkey_filename) | |||
except BaseException as e: |
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 had this first fwiw, but switched to the more general one because I assumed open and read here could throw some kind of I/O exception?
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, it's merged already 🎉 probably not too important in any case. Thanks for seeing this through!
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 had this first fwiw, but switched to the more general one because I assumed open and read here could throw some kind of I/O exception?
They could, but catching BaseExeption
is usually not great. We check that the file exists above so I suspect that wouldn't happen...although I guess there could be permission issues.
#176
Needs a change in a dependency so had to fork it for now, have a PR to merge it upstream at web-push-libs/pywebpush#133This has been merged and released upstream.