-
Notifications
You must be signed in to change notification settings - Fork 372
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
Remove service worker #936
Conversation
Updated Lighthouse report for the changes in this PR:
Tested with Lighthouse version: 4.1.0 |
What happens to all the people have installed this on desktops and home screens? |
Updated Lighthouse report for the changes in this PR:
Tested with Lighthouse version: 4.1.0 |
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.
Change LGTM. My operating assumption would be that the json fetch would end up in the user's local cache. Are we properly setting the Cache-Control value so that happens?
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.
Discussed caching offline.
I think that the icon disappears or it becomes a plain bookmark icon. The install button was only ever offered on mobile, so I don't think many users have it. I added to my phone's home screen and then updated the staging server. The icon is still there, but it might disappear after 24 hours. I looked through various specs and docs and failed to find what the expected behavior is. |
While service worker is awesome, it is not needed for this app. Removing it will simplify debugging of JS and CSS issues.
This will resolve issue #892.