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

Set service worker timeout #973

Merged
merged 2 commits into from
Mar 1, 2019
Merged

Set service worker timeout #973

merged 2 commits into from
Mar 1, 2019

Conversation

gfodor
Copy link
Contributor

@gfodor gfodor commented Feb 28, 2019

This adds a 5s timeout to the registration of the service worker completing. I thought I had all the relevant error handling in place, but it seems that in some cases the browser is not completing the promise for the registration, at all, when testing on a mobile device on a LAN. I am not positive what is going on, it seems related to the service worker loading over a slow LAN, but this timeout seems sane anyway (and the result is that the checkbox will just be hidden for notifications.)

@gfodor gfodor requested a review from brianpeiris February 28, 2019 16:53
Copy link
Contributor

@brianpeiris brianpeiris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable. MDN docs do say that serviceWorker.ready never rejects and may "eventually" resolve. It's a bit unfortunate that we are potentially introducing a 5 second loading delay here.
May be nice to at least log a warning when the timeout happens.

@gfodor gfodor merged commit 7bd40dc into master Mar 1, 2019
@hubs-build-size-bot
Copy link

hubs-build-size-bot commented Mar 1, 2019

.html +34 bytes        🔼 /hub.html                                                    +34 bytes        🔼
.ico  0 bytes         
.png  0 bytes         
.js   +248 bytes       🔼 /assets/js/admindeps-[hash].js                               +1 bytes         🔼
/assets/js/hub-[hash].js                                     +246 bytes       🔼
/assets/js/vendor-[hash].js                                  +1 bytes         🔼
.glb  0 bytes         
.jpg  0 bytes         
.svg  0 bytes         
.map  +1,224 bytes     🔼 /assets/js/engine-[hash].js.map                              -17 bytes       
/assets/js/hub-[hash].js.map                                 +1,241 bytes     🔼
.mp3  0 bytes         
.wav  0 bytes         
.webm 0 bytes         
.ogg  0 bytes         
.css  0 bytes         
.mp4  0 bytes         

@johnshaughnessy
Copy link
Contributor

Ah nice, I was going to ask about this. Had done something similar locally.
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants