-
Notifications
You must be signed in to change notification settings - Fork 1k
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
feat(cli/web): pwa push notifications #1892
Conversation
ab8563b
to
69b683b
Compare
Have you planned to make a pull request on the capacitor depot? |
Sorry, not sure if I understand. This PR is already for the capacitor repository, unless I'm missing something? |
Yes excuse me, my question is: When will be validated the pull request. Because it's an important feature. Although I know it does not depend on you but on ionic team |
It's a pretty significant change and thus requires a time-consuming code and design review, so I can understand how it's taking a backseat. That being said it has been sitting in the PR queue for a few months. Perhaps @mlynch or @jcesarmobile can provide an estimate when this PR may be looked at? Looking forward to the discussion here and getting the best solution checked-in. I can vouch that in my personal projects the PR is still working fine with the latest capacitor builds. |
Updated the PR to include a change in copy.ts to handle data messages in our service worker when PWA is running in the background. See StackOverflow article for info on how to send such messages. |
been monitoring this as well, looks like we gonna wait a little bit longer... |
Thanks @CFT-Chris, any update on the approval process? All tests passed, no conflicts, why is it stalled? |
Hey all, thank so much for this PR. Given that this adds a dependency for firebase on the web I think this is more appropriate as a third party plugin that we can link to. We need to support the broadest set of functionality wherever possible, and we can't add specific Google/Firebase libraries on the web side where technically they aren't required (unlike Android where they are hard to avoid). Do you have any thoughts on moving this to a third party firebase PWA plugin for Capacitor, and then also about how we could do a "plain" push plugin for web for Capacitor? Also I realize this is frustrating. To be more clear about our goals here re: third party libs I've added some more language to the contributing guide https://github.com/ionic-team/capacitor/blob/master/.github/CONTRIBUTING.md |
@mlynch, thanks for the discussion. I agree completely that the third-party libs (namely Firebase) should be separated out, to avoid having version maintenance of third-party libraries. Perhaps I could extend the functionality I wrote for cli/web to be able to include multiple third-party service workers rather than just a single service worker. Capacitor would act as an aggregator of all service workers needed, and ultimately the service worker produced by CLI can be included in PWAs. A new plugin/project would manage a Firebase service worker compatible with Capacitor. Then it's up to the developer to make sure that all the necessary service workers are imported (and in the correct order), supplying that list to capacitor.config.json so that the CLI can aggregate them into a single service-worker. As for the "lodash" import and regarding what was added to Contributing.md, "lodash" is not exported to any web projects. It is used internally in CLI copy code to merge capacitor.config.json with the default configuration. I think that is still a convenient change to keep from my PR. |
@mlynch, I've updated the pull request and factored out all the Firebase related code. Almost everything I wrote in my original comment doesn't apply to this new set of changes, except for the part regarding Capacitor's own service-worker, as I will describe at the end of this comment. I've created a new plugin capacitor-pwa-firebase-msg which fills in the missing gap for PushNotifications on the "web" platform if one decides to go the Firebase route. This plugin is contingent though on the changes I'm proposing in this pull request. What's needed are the CLI modifications to run the npm script "precapcopyweb" where present in the package.json of any web plugins during the copy operation, e.g. With this solution, it is possible to not be "stuck" using Firebase. Other developers can develop plugins in a similar manner to support other third-party back-ends for the "web" platform. This PR will also open up the possibility of supporting an "aggregate" service-worker. The "npx cap copy web" operation as already described allows the plugins to copy assets to the www directory via "precapcopyweb" npm scripts, and then at the end of the operation, all the service-workers can be bundled into a single service-worker for the PWA. This is done through the addition of two capacitor.config.json options under Capacitor is now free too to add its own service-worker code for the "web" platform and its core plugins/API. |
@stewwan I'm a little hazy on this, but would this make sense in FCM Plugin? |
@dwieeb @stewwan I have no problem merging my plugin with the FCM plugin to enable support for PWAs if that's desired. This change creates a way to aggregate all needed service worker code into a single service worker using Capacitor commands. The So for this particular plugin already published, capacitor-pwa-firebase-msg, which requires service worker code to work, if combined with this PR, it would be a matter of just running 'npx cap copy web' with this plugin npm installed to generate a final service worker. |
The problem with copying web specific files (and capacitor.config.json) into the www folder is that the content of www is copied to ios and android when their respective copy commands are run, so that will cause the |
I like that idea of populating platforms/web. Shipping the "www" folder for PWAs didn't allow incremental, separated changes to mobile platforms without affecting the shipped PWA product unless that folder was manually copied. I've been lazily shipping PWAs by keeping the "www" folder in the repo (commenting it out of .gitignore) which thus runs into the above problem. |
I like that idea as well. We can provide PWA manifest files, service workers, etc. In any case, we may want to think about this a little more. This will likely be something we re-address in Capacitor 3. |
@CFT-Chris Thanks again for this, we will address this in Capacitor 3 as discussed. |
Basic handling of Firebase messages (background and foreground) for PWA.
Implements following in Push Notifications API:
Creates framework for Capacitor's own service worker. Configuration of service worker in
capacitor.config.json
. Service worker is copied towww
vianpx cap copy web
(same manner asbundledWebRuntime
).If
firebaseConfig
provided underserviceWorker
in config, then necessary Firebase service worker code is copied too and referenced in Capacitor's service worker.capacitor.config.json
itself is copied towww
as well because we need the configuration at runtime to initialize Firebase.If
combineOtherWorker
is provided, then Capacitor's service worker will reference that script at the end.It is up to the developer to make sure Capacitor's service worker is used (see doc changes). At the moment, Capacitor's Service worker will likely just be a combination Firebase service worker code with existing service worker code like Angular's service worker,
ngsw-worker.js
.The order of code in Capacitor's service worker matters. The Firebase code needs to precede any other service worker code if it is to handle Firebase push events properly. The default Angular service worker, for example, breaks background push events if it gets the push event at all. That's why the
event.stopImmediatePropagation()
is necessary in Capacitor's service worker.In
config.ts
, I changedObject.assign
to lodash's deep merge of objects so that we can retain the default nested values of the config object. I don't see this change affecting any other Capacitor features.Note about Firebase message payloads to PWA:
For Firebase messages received in background, it is necessary to populate the
notification
in the payload withclick_action
, whereclick_action
is the absolute URL to the PWA page expected to be open whenpushNotificationActionPerformed
is handled. Even though the Firebase spec implies that we could overrideclick_action
viawebpush
in the payload instead, in my experience I could never get background push notifications to work properly (i.e. restoring the PWA from background on click or spawning a new instance if not running) usingwebpush
. This means that on the back-end, the push payloads need to know whether it is pushing to PWA or Android or iOS so that theclick_action
is tailored to the correct platform. I couldn't solve this shortcoming via the code I'm submitting here and could only do so via back-end. It would be great if someone could inform me what I may be missing to make the more robust payloads (i.e.webpush
,android
andapns
overrides) work.Fixes: #1282