-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
WIP: HTML5 Push Notifications #10884
Conversation
This is to avoid a circular import. The model triggers a web push, and the web push deletes old expired pushes.
No, I'm not sure why crypto has updated slightly during `go get`...
Can you resolve the conflict - Ill look at it afterwards :) |
@@ -131,7 +131,7 @@ Values containing `#` or `;` must be quoted using `` ` `` or `"""`. | |||
- `REACTIONS`: All available reactions. Allow users react with different emoji's. | |||
- `DEFAULT_SHOW_FULL_NAME`: **false**: Whether the full name of the users should be shown where possible. If the full name isn't set, the username will be used. | |||
- `SEARCH_REPO_DESCRIPTION`: **true**: Whether to search within description at repository search on explore page. | |||
- `USE_SERVICE_WORKER`: **true**: Whether to enable a Service Worker to cache frontend assets. | |||
- `USE_SERVICE_WORKER`: **true**: Whether to enable a Service Worker to cache frontend assets and enable push notifications on supported browsers. |
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.
Don't piggy-back on the service-worker pref, these are two distinct features. Add a new one, like USE_PUSH_NOTIFICATIONS
.
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.
True - I'll pop a note saying the service worker is required as well.
@@ -289,6 +289,8 @@ set name for unique queues. Individual queues will default to | |||
|
|||
- `INSTALL_LOCK`: **false**: Disallow access to the install page. | |||
- `SECRET_KEY`: **\<random at every install\>**: Global secret key. This should be changed. | |||
- `WEB_PUSH_PUBLIC_KEY`: **\<random at every install\>**: VAPID key pair used for Web Push notifications | |||
- `WEB_PUSH_PRIVATE_KEY`: **\<random at every install\>**: VAPID key pair used for Web Push notifications |
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.
PUSH_NOTIFICATIONS_PUBLIC_KEY
and PUSH_NOTIFICATIONS_PRIVATE_KEY
would be more better imho.
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.
In the codebase I've tried to be consistent with 'Web Push', but I could change the user-facing config quite easily
350c7ad
to
5dafc6c
Compare
Cheers, but it's insistent on putting the |
this could be indeed a windows only issue |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 months. Thank you for your contributions. |
Pleace resolve conflicts |
Serviceworker implementation has changed significantly since this PR as it's now handled by workbox so this will need quite a few updates. Also I think I'm not generally against adding push notifications but it needs to be integrated with our existing EventSource-based notifications, probably sharing much code with that implementation. |
Hi all, Yep, this one certainly needs updating! I was testing the water with how push notification implementations work for sure. It's been a while since I've had time to dig through Gitea's codebase but I'll try and block some time this weekend to have a play. I might restart this from the latest commit from master. I haven't really touched Workbox before, but I'm guessing I'll be able to add This might take quite a while in the grand scheme of things, and we'd have to accept it wouldn't work (or do we try to fallback to AJAX as in #10961?) in IE or Safari. Thanks. |
@jamesorlakin IE support was droped in v1.13 |
Even better 👍 |
I imagine the new implementation can re-use the |
I'm pretty sure they don't as I have implemented push notifications for desktop and mobile a while ago using the standard APIs. Edit: https://caniuse.com/#feat=push-api also shows support for desktop browsers. |
I guess fallback to web notification is a requirement because there are scenarios where serviceworker will be disabled (either by configuration or when the user is in private browsing mode (which may disable serviceworker in some circumstances)). Maybe there is a third-party module that can handle such a fallback. |
There is still the default notification system (email, just the bell in the top right of the web interface)
I think then a fallback is not intended either (or do you mean disabled in the browser?)
I think then you can't even enable the push notifications and I don't think this is the way most people who want push notifications use their browser.
Unfortunately I don't think so as the technology is quite different (Web Notifications is just showing a Notification and Push Notification is also getting updates to the browser) So in my personal opinion I would see Push Notifications as a progressive enhancement so I don't think a fallback is required. This would also simplify the implementation. |
We do have SSE/Eventsource implemented for server-side push things. Currently it's only used to update the notification count. We probably will want to switch this to websockets because SSE has some quirks and limitations. What I mean is that web notifications would still be possible even without a service worker available while push notifications require it, but I guess we don't need to handle it as it would complicate the implementation (e.g. need to handle multiple tabs racing to push the same notification etc). |
Correct me if I'm wrong here, but I thought that was handled by an AJAX timer? I think for now we should stick to feature discovery (does the browser support SWs, push, etc) and only show the option if the service worker is enabled in the config. Still showing notifications outside of Web Push is a nice idea, but we need to make it clear this fallback mode would not provide notifications if no Gitea tab is open. |
See https://github.com/go-gitea/gitea/blob/master/web_src/js/features/notification.js#L30, it's using EventSource and falls back to ajax, at least that's the plan.
I would make a second option and only enable push notifications if both are enabled, that way one can still benefit from SW caching while keeping notifications off if desired. |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 months. Thank you for your contributions. |
This pull request has been automatically closed because of inactivity. You can re-open it if needed. |
Is this ever gonna land? :# |
Afraid it's unlikely from me - apologies to everyone for leaving this in an unfinished state. I absolutely adore all the effort put into this project and wish for it to thrive for years to come. For anybody looking to take this on feel free to copy any of the code thus far as your own. If there's any questions or a later PR feel free to tag me if you'd like. Cheers! |
Edit: This PR is currently being rewritten. I'll force push it when I'm done but feel free to see my
pushNotificationsNew
branch in the meantime.Fixes #10804, #9327.
This is a 'mostly there but unpolished' PR to implement push notifications using the Web Push and Notification API, powered by the webpush-go library. I feel this is a very useful piece of functionality to be productive, especially if emails aren't configured.
To give an overview:
Whenever a notification model is created or updated we send a push notification:
As a result the service worker is required to be enabled - it's quite key! Having this lays the foundation for live messaging outside of notifications later on: updating the bell icon, informing the user of new activity while viewing a PR, etc.
This is WIP because I have some things outstanding/discussing:
Decouple the sending of pushes from the actual web request that triggered it. Would a simple goroutine be sufficient? Let's say a push service took 10s... ouch!Refactor web push to be a module using the notification service entrypoint, rather than hijacking events on the notification model.ini
config namesAnd as always, even minor nits are welcome! A lot of the logic is in the models, but I need it to automatically delete old hooks and react to notifications. It was a module but I caused an import cycle... 🤦♂