-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Initialize groundwork for PWA support #9204
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
Conversation
|
@mtanmaykapil is attempting to deploy a commit to the eventyay Team on Vercel. A member of the Team first needs to authorize it. |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds initial PWA groundwork by registering a service worker when the app loads, without changing existing application behavior. Sequence diagram for service worker registration on app loadsequenceDiagram
actor User
participant Browser
participant App
participant ServiceWorker
User ->> Browser: Open application URL
Browser ->> App: Load app.js
App ->> Browser: Attach window load listener
Browser -->> User: Render initial UI
Browser ->> Browser: window load event fires
Browser ->> ServiceWorker: navigator.serviceWorker.register(/service-worker.js)
ServiceWorker -->> Browser: Registration promise resolves/rejects
Browser -->> App: Service worker registration result (unused in this PR)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
We don't accept pull requests from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes - here's some feedback:
- The global
if ('serviceWorker' in navigator)guard will throw in non-browser environments (e.g., FastBoot or SSR); consider usingtypeof navigator !== 'undefined' && 'serviceWorker' in navigatoror a similar runtime environment check before accessingnavigator. - You may want to restrict service worker registration to production (or a specific environment/flag) to avoid interfering with local development behavior and caching while iterating.
- Consider adding basic error handling or logging around
navigator.serviceWorker.register('/service-worker.js')so that registration failures are surfaced and easier to debug during rollout.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The global `if ('serviceWorker' in navigator)` guard will throw in non-browser environments (e.g., FastBoot or SSR); consider using `typeof navigator !== 'undefined' && 'serviceWorker' in navigator` or a similar runtime environment check before accessing `navigator`.
- You may want to restrict service worker registration to production (or a specific environment/flag) to avoid interfering with local development behavior and caching while iterating.
- Consider adding basic error handling or logging around `navigator.serviceWorker.register('/service-worker.js')` so that registration failures are surfaced and easier to debug during rollout.
## Individual Comments
### Comment 1
<location> `app/app.js:18-19` </location>
<code_context>
export default App;
+
+if ('serviceWorker' in navigator) {
+ window.addEventListener('load', () => {
+ navigator.serviceWorker.register('/service-worker.js');
+ });
+}
</code_context>
<issue_to_address>
**suggestion:** Consider adding basic error handling around service worker registration.
If registration fails (e.g., invalid script, wrong path, non-HTTPS), the error is currently ignored. Consider handling the returned promise (via `.then`/`.catch` or `async/await`) and logging/reporting failures:
```js
window.addEventListener('load', () => {
navigator.serviceWorker
.register('/service-worker.js')
.catch((err) => {
// log or report err
});
});
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| window.addEventListener('load', () => { | ||
| navigator.serviceWorker.register('/service-worker.js'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Consider adding basic error handling around service worker registration.
If registration fails (e.g., invalid script, wrong path, non-HTTPS), the error is currently ignored. Consider handling the returned promise (via .then/.catch or async/await) and logging/reporting failures:
window.addEventListener('load', () => {
navigator.serviceWorker
.register('/service-worker.js')
.catch((err) => {
// log or report err
});
});
This PR adds the initial groundwork for Progressive Web App (PWA) support.
This is an incremental change, and further improvements (manifest configuration and service worker enhancements) will be added based on feedback.
Summary by Sourcery
Enhancements: