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

feat(cli/web): pwa push notifications #1892

Closed
wants to merge 21 commits into from

Conversation

CFT-Chris
Copy link
Contributor

Basic handling of Firebase messages (background and foreground) for PWA.

Implements following in Push Notifications API:

register()
addListener('registration')
addListener('pushNotificationReceived')
addListener('pushNotificationActionPerformed')

Creates framework for Capacitor's own service worker. Configuration of service worker in capacitor.config.json. Service worker is copied to www via npx cap copy web (same manner as bundledWebRuntime).

If firebaseConfig provided under serviceWorker in config, then necessary Firebase service worker code is copied too and referenced in Capacitor's service worker.

capacitor.config.json itself is copied to www 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 changed Object.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 with click_action, where click_action is the absolute URL to the PWA page expected to be open when pushNotificationActionPerformed is handled. Even though the Firebase spec implies that we could override click_action via webpush 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) using webpush. 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 the click_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 and apns overrides) work.

Fixes: #1282

@KyDenZ
Copy link
Contributor

KyDenZ commented Oct 8, 2019

Have you planned to make a pull request on the capacitor depot?

@CFT-Chris
Copy link
Contributor Author

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?

@KyDenZ
Copy link
Contributor

KyDenZ commented Oct 8, 2019

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

@CFT-Chris
Copy link
Contributor Author

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.

@CFT-Chris
Copy link
Contributor Author

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.

@jcuervo
Copy link

jcuervo commented Dec 6, 2019

been monitoring this as well, looks like we gonna wait a little bit longer...

@jmdavid
Copy link

jmdavid commented Dec 16, 2019

Thanks @CFT-Chris, any update on the approval process? All tests passed, no conflicts, why is it stalled?

@jcesarmobile jcesarmobile added the needs discussion decisions must be made before working on it label Jan 23, 2020
@mlynch
Copy link
Contributor

mlynch commented Mar 3, 2020

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

@CFT-Chris
Copy link
Contributor Author

@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.

@CFT-Chris
Copy link
Contributor Author

@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. npx cap copy web. This preprocessing is required to generate the necessary service worker and supporting files to the www directory in order to make push notifications with Firebase work.

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 serviceWorker: name and combineWorkers.

Capacitor is now free too to add its own service-worker code for the "web" platform and its core plugins/API.

@imhoffd imhoffd self-assigned this Jun 30, 2020
@imhoffd
Copy link
Contributor

imhoffd commented Jun 30, 2020

@stewwan I'm a little hazy on this, but would this make sense in FCM Plugin?

@stewones
Copy link
Contributor

stewones commented Jul 1, 2020

@CFT-Chris
Copy link
Contributor Author

@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 copy web command looks into all Capacitor plugins and retrieves their service worker code, in addition to any service worker code manually specified in capacitor.config.json, and stitches them together to be used by the Capacitor-powered PWA.

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.

@jcesarmobile
Copy link
Member

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 capacitor.config.json to be duplicated in native projects and native projects getting web specific files.
So, maybe it's time to create a "web" folder that gets the content of www and also web specific files?

@CFT-Chris
Copy link
Contributor Author

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.

@imhoffd
Copy link
Contributor

imhoffd commented Jul 1, 2020

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.

@imhoffd
Copy link
Contributor

imhoffd commented Jul 9, 2020

@CFT-Chris Thanks again for this, we will address this in Capacitor 3 as discussed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs discussion decisions must be made before working on it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Push notifications in PWA
8 participants