Skip to content
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 delivery #297

Closed
wants to merge 13 commits into from
Closed

WebPush delivery #297

wants to merge 13 commits into from

Conversation

jbennett
Copy link

@jbennett jbennett commented Jul 6, 2023

This adds support for a WebPush delivery method including some sample javascript to make it all work.

Looking for any initial feedback. If you are good in theory, I'll:

  • add the appropriate docs
  • use active record encryption option

5 minute video of setting up from scratch for reference: https://youtu.be/d-oXK7HoRuk

@jbennett jbennett mentioned this pull request Jul 6, 2023
@jbennett
Copy link
Author

jbennett commented Jul 8, 2023

Added out-of-the-box support for encrypting keys via Active Record Encryption and what method is used on the notification to supply the data.

From a functional point-of–view, does this give enough for getting started?

@jbennett jbennett marked this pull request as ready for review July 8, 2023 04:49
@DmitrySychev
Copy link

Test driving this PR. Were you able to setup push on apple devices with safari? On my end, ios 17 beta3, with notifications enabled and ssl certs setup.

JS doesn't wait for user input in the request prompt. It skips to the notification declined bit of code immediately.

        Notification.requestPermission()
            .then((permission) => {
                if (permission === "granted") {
                  setupSubscription()
                } else {
                    alert("Notifications declined")
                }
            })
            .catch(error => console.log("Notifications error", error))
            .finally(() => notificationsButton.style.display = "none")
    })

@Tonksthebear
Copy link

I just set this up on a new project I'm working on and the only gotcha I had to work around was adding a manifest.json with display: "standalone". After adding my site to the Home Screen with that manifest, Notification became present in the iOS web console and thus this worked.

I was incredibly impressed with how well this all worked. Only suggestion I'd have is adding a default manifest.json if it's not already present. If I did it right, that involved:

  1. Add manifest.json to /app/assets/config
  2. Add //= link manifest.json to /app/assets/config/manifest.js
  3. Add <link rel="manifest" href="<%= asset_path 'manifest.json' %>" data-turbo-track="reload"> to /app/views/layouts/application.html.erb

This was a great addition though, love that it already accounts for turbo too

@jbennett
Copy link
Author

Great catch. I was still loading a cached manifest I think. I'll work up an update.

@jbennett
Copy link
Author

@Tonksthebear Added in a app.webmanifest (that's the newer version of manifest.json).

@DmitrySychev Working on this. I've added some notes about setting up permissions with the self signed certificate, but still running into problems here. Not sure what's different from my other setup. For you're quick reference:

3. Install certificate on mobile device
  1. Download and add localhost.crt onto your mobile device
  2. Install and trust [iOS](https://support.apple.com/en-us/HT204477) [Android](https://proxyman.io/posts/2020-09-29-Install-And-Trust-Self-Signed-Certificate-On-Android-11)
  3. Skipping this will cause the notification prompt to always be denied

Let me know if that works for you

@DmitrySychev
Copy link

@Tonksthebear Added in a app.webmanifest (that's the newer version of manifest.json).

@DmitrySychev Working on this. I've added some notes about setting up permissions with the self signed certificate, but still running into problems here. Not sure what's different from my other setup. For you're quick reference:

3. Install certificate on mobile device
  1. Download and add localhost.crt onto your mobile device
  2. Install and trust [iOS](https://support.apple.com/en-us/HT204477) [Android](https://proxyman.io/posts/2020-09-29-Install-And-Trust-Self-Signed-Certificate-On-Android-11)
  3. Skipping this will cause the notification prompt to always be denied

Let me know if that works for you

The missing bits were two fold. First as @Tonksthebear suggested, it was the manifest.json, second, it was adding the add to homescreen like a pwa. After that it looks like we're all good and it's working as intended. Fantastic work! Thank you.

One other thought on my end. I'm not sure how I feel about requiring the use of encrypted AR. If the user has not made that decision, I don't feel like we should be making that for them.

@jbennett
Copy link
Author

I think it's good to not store the info in plain text by default.

There is an option to not generate it and it's easy to remove you you don't want it.

@manglav
Copy link

manglav commented Sep 1, 2023

Question - is there anything pending for this PR? I'd love to be able to use webpush straight from noticed.

@jbennett
Copy link
Author

jbennett commented Sep 1, 2023

I'm using this in production so I think its good.

@DmitrySychev
Copy link

Heya, @excid3, is there anything we need to do to merge this one? I've been using it live as well, and it has been performing as intended.

Unless you have other ideas as far as webpush integrations. Thank you!

@excid3
Copy link
Owner

excid3 commented Dec 24, 2023

Just a heads up, there's a huge rewrite coming in #313.

Re-reviewing this, I'm excited for WebPush support, but also seems like there's a lot of extras to it so what if this became a separate gem?

@jbennett
Copy link
Author

I saw the 2.0 stuff when checking on in this PR. I'll keep an eye on it for sure.

Separate gem isn't a bad idea at all. I've started pulling it out into an engine over here: https://github.com/jbennett/noticed-web_push.

Depending on how your 2.0 goes, I might win a record from going from v0 to v1 to v2 lol

@excid3
Copy link
Owner

excid3 commented Dec 28, 2023

Sounds like DHH wants to integrate Web Push in Rails 8! rails/rails#50454

@DmitrySychev
Copy link

Sounds like DHH wants to integrate Web Push in Rails 8! rails/rails#50454

Woah, not a turn of events I was expecting. That's awesome! Thanks for sharing, Chris.

@jbennett
Copy link
Author

Lol. I just switched 3 prod projects from this fork to my new engine. Ok, new record from v0 -> v1 -> v2? -> sherlocked

@DmitrySychev
Copy link

Lol. I just switched 3 prod projects from this fork to my new engine. Ok, new record from v0 -> v1 -> v2? -> sherlocked

To be honest, I think there's still quite a bit of value in your work here. Rails 8 is still a ways away, and we're not sure if this is actually going to happen. Right now, it's a conversation, while your work is already available and can be applied immediately.

@jbennett
Copy link
Author

Ok, I think I have a reasonable beta of an engine. I'm handling most of the weird service worker stuff etc so you have just add the gem, install, and add the delivery method to your existing notifications.

You can see the repo here: https://github.com/jbennett/noticed-web_push
and a 2 minute install video here: https://www.youtube.com/watch?v=-9KWx7Pj5AM

I need to do a little work on the sample web_push javascript but it's mostly there as a placeholder for you to get started with.

Would love any feedback

@AxelTheGerman
Copy link

Started reading the old issues and seeing a long running PR made me sad but this took quite a turn in the last few weeks.

Going to give noticed-web_push a go 🚀

Definitely would make sense to leave it as separate gem for now as it does add a lot of complexity that is not needed when using only the other delivery methods - but IMHO would deserve a clear shout out in the list of delivery methods.

Additionally if it does land in Rails it hopefully supports all the same features and could be replaced long-term.

@excid3 excid3 closed this Jan 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants