Open AxelTheGerman opened 11 months ago
Thanks so much for your input. I'll be working through this as I think about v2 in light of the stuff that DHH is proposing for Rails 8.
I'm currently thinking about splitting this into two a PWA focused engine that will be conceptually consistent with what the Rails 8 changes will be and a noticed-web_push that builds on that for noticed. I see the first as being a stopgap and will go away once you are on Rails 8.
What are your feelings on that?
Hi Jonathan,
Thanks for tackling this web push topic it really is quite a beast, hence DHH thinks it's worth going after!
Just wanted to let you know a couple of gotchas I ran into when installing the gem.
1. DB migration
My users table has UUIDs as primary keys and I had to change that in the migration - common issue and actually happened on
noticed
itself as well.2. installer
It failed but I will break out the real issues separately.
``` $ rails g noticed:web_push:install 2024-01-06T03:09:24.983Z pid=34373 tid=143l INFO: Sidekiq 7.2.0 connecting to Redis with options {:size=>10, :pool_name=>"internal", :url=>nil} run yarn add github:jbennett/noticed-web_push from "." yarn add v1.22.21 info No lockfile found. [1/4] Resolving packages... [2/4] Fetching packages... [3/4] Linking dependencies... [4/4] Building fresh packages... success Saved lockfile. success Saved 1 new dependency. info Direct dependencies └─ noticed-web_push@0.1.0 info All dependencies └─ noticed-web_push@0.1.0 Done in 1.64s. append app/javascript/application.js run touch app/javascript/service_worker.js from "." append app/javascript/service_worker.js append config/initializers/assets.rb /home/axel/.gem/ruby/3.2.2/bundler/gems/noticed-web_push-132116a5c22b/lib/generators/noticed/web_push/templates/app.webmanifest.erb.tt:8:in `template': undefined method `image_path' for #So I had to do the following manually:
has_many
to usersroutes.rb
3. yarn
As I'm not using yarn to pull in your scripts, I copied the files from https://github.com/jbennett/noticed-web_push/tree/main/app/assets/javascripts/noticed/web_push to my own
app/javascript
folder.I'm not a JS guy so not sure how this can be packaged better - or if it needs to, but I'm using importmaps which is kind of the new default for Rails, so should somehow work with this (and maybe have an option in the installer OR detect whether yarn/npm is used).
Also had to change the imports in the 2 JS files from
from "noticed-web_push/web_push"
to `from "./web_push/web_push" to resolve to my local copies.4. Active record encryption
Getting the following error when trying to save the web push subscription:
Makes sense to save this encrypted but if it is a requirement it could maybe be checked in the install script - or it could be made optional (though compromising on security is not good).
Noticed does require rails but only 5.2+ I think, so technically this gem might have to require a more recent rails version itself.
5. ApplicationController
It is weird as it seems you have
Noticed::WebPush::ApplicationController
but it seems like the controllers are inheriting from my appsApplicationController
as I do have abefore_action :require_logged_in_user!
which I selectively skip where applicable and it catches for all the gem mounted controller actions.6. Javascript, again
I actually had to also copy the
service_worker.js
(copied from the gem) content into mysevice_worker.js
file as, at least with import maps, I can't just have an "import" statement.Again I like the plug & play approach but some of that JS might want to be customized as well - perhaps
web_push.start
is not the most elegant API/DSL for this.7. save vs setup subscription
I had to change the
Notification.permission == "granted"
case to check for existing service worker registration.As I hit some snags setting everything up (issues with my service worker especially) it happened that Notification access was granted but the service worker was never set up - and
saveSubscription
would "hang" forever onconst registration = await navigator.serviceWorker.ready
8. I would like to customize the "Enable notifications" button
It relies on it's ID so I can probably just add a button with that ID anywhere I like though.
What I really liked though was:
That was a lot and I might have forgotten some!
But I got it working :rocket: Now I still want to clean things up a little bit. It's quite tricky as there are so many moving pieces especially with the JS and all the routes - I also don't know of any clean ways to pass configuration such as routes from Ruby to JS (without dragging them through the HTML).