Enhance Web Push Example with Non-Interactive Subscription & Auto-Activation in Service Worker #1410
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
GAURAVCHAWLA1808
force-pushed
the
push-notification-fix
branch
from
a7acc80 to
d0ff324
Compare
GAURAVCHAWLA1808
changed the title
|
@patrickkettner, is this one you would like to look at? If not, feel free to assign it to me. |
|
Hi @oliverdunk , I would appreciate your guidance on this PR and any improvements I can make. Your insights would be valuable in ensuring everything aligns well with the project. Additionally, I’m eager to contribute more—could you suggest other issues or areas where I can help? Looking forward to your guidance. |
There was a problem hiding this comment.
Thanks for opening this, and sorry for the delay leaving a review. Generally we want PRs to be well scoped and solve a single problem - in this case it's unclear what this PR's main goal is, as it does a lot of things:
- Changes logging
- Introduces a popup
- Automatically subscribes on service worker startup
- Significantly changes the README
- Changes how the API key is provided
It would be great if you could choose one of those things and make the PR focused on that. We can then definitely review work on the others in additional PRs :)
| @@ -1,21 +1,137 @@ | |||
| # Service worker with push notification | |||
| # Service Worker with Push Notification | |||
There was a problem hiding this comment.
Thanks for taking another look at the README. There are certainly some readability improvements, but the changes also take us further away from the template that we try to use for all samples: https://github.com/GoogleChrome/chrome-extensions-samples/blob/main/README-template.md
A few examples:
- "This sample demonstrates" is a common phrase we use at the start of every sample.
- We don't usually have a changelog in the README.
Could you take a look and try to make this closer to the template and avoid any unnecessary changes that don't significantly improve readability or add information that a new developer opening this sample would need?
There was a problem hiding this comment.
Sorry for the in convenience , I'll align Readme according to template .
| git clone https://github.com/GoogleChrome/chrome-extensions-samples.git | ||
| cd chrome-extensions-samples/functional-samples/cookbook.push | ||
| ``` | ||
| 2. Open `background.js` and **replace** `APPLICATION_SERVER_PUBLIC_KEY` with your public key from the web push test server. |
There was a problem hiding this comment.
APPLICATION_SERVER_PUBLIC_KEY no longer exists in this version. I think it should do, though - can we revert that change?
There was a problem hiding this comment.
You're right! That was removed unintentionally. I'll revert that change and ensure the instructions remain clear for developers.
| console.log("✅ Background service worker is running!"); | ||
|
|
||
| // Install event | ||
| self.addEventListener("install", () => { |
There was a problem hiding this comment.
Is there a reason for this? Otherwise, I would prefer to remove it as we generally don't want to make developers think they need to listen to service worker lifecycle events.
There was a problem hiding this comment.
The install event was added for debugging purposes, but I agree that it might be misleading. I'll remove it.
| // Activate event | ||
| self.addEventListener("activate", (event) => { | ||
| console.log("✅ Service Worker Activated."); | ||
| event.waitUntil(subscribeUserVisibleOnlyFalse()); |
There was a problem hiding this comment.
Would this work in a chrome.runtime.onInstalled listener instead?
There was a problem hiding this comment.
Good point! I'll move this logic to a chrome.runtime.onInstalled listener and test if it works as expected.
| }, | ||
| "permissions": ["notifications"], | ||
| "host_permissions": ["https://*/*"], | ||
| "gcm_sender_id": "103953800507" |
There was a problem hiding this comment.
We don't need this , this was added as I was testing FCM ( Firebase Cloud Messaging ) for sending push notifications which needs "gcm_sender_id" .
| } | ||
| }, | ||
| "permissions": ["notifications"], | ||
| "host_permissions": ["https://*/*"], |
There was a problem hiding this comment.
Why do we need host permissions?
There was a problem hiding this comment.
This was added to allow the extension to interact with specific web pages , If it’s not essential, I can remove or restrict it to specific domains.
Thanks for your feedback, Oliver! I understand the concern regarding the PR scope. I'll separate the changes into multiple PRs, each focusing on a single issue . |
This PR fixes Issue #1408 by automating the push subscription process in the cookbook.push extension. Previously, users had to manually subscribe via the Service Worker DevTools. This update ensures the subscription is created automatically when the Service Worker activates.
Changes Implemented
background.js
Added event.waitUntil(subscribeUserVisibleOnlyFalse()); in the activate event to auto-subscribe users.
Improved logging for debugging.
Ensured userVisibleOnly = true for push notifications.
manifest.json
Added "gcm_sender_id": "103953800507" for better push compatibility.
popup.js
Improved error handling for subscription failures.
Added clear logs when subscribing/unsubscribing.
README.md
Updated instructions to include automatic subscription.
Added a template for send-push.js to help test push messages.
This update ensures a fully functional, real-world web push example without requiring manual DevTools interaction.
Fixes #1408