-
Notifications
You must be signed in to change notification settings - Fork 681
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
Network-first service worker caches #2515
Conversation
e492c21
to
98670e3
Compare
f9dac3c
to
439a798
Compare
439a798
to
89850f3
Compare
Been running this without problems. |
client/views/windows/settings.tpl
Outdated
@@ -14,6 +14,11 @@ | |||
</div> | |||
|
|||
<div class="row"> | |||
<div class="col-sm-12" id="webAppInstallButton" style="display:none"> |
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.
Class instead of inline style?
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.
Switched to hidden
attribute
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.
Or hidden
CSS class? 🤷♂️
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.
The hidden attribute is in standard, so makes sense to use that.
89850f3
to
ab1ce4e
Compare
ab1ce4e
to
28df906
Compare
I need to play with this a bit but code looks good to me. I have extremely low confidence that this works as expected on iOS (though who knows, maybe it does), so @McInkay if you want to test it on Android as well that would be great. |
client/js/webpush.js
Outdated
@@ -9,6 +9,10 @@ let clientSubscribed = null; | |||
let applicationServerKey; | |||
|
|||
if ("serviceWorker" in navigator) { | |||
window.addEventListener("beforeinstallprompt", (e) => { | |||
e.prompt(); |
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.
What does this do?
@@ -44,7 +44,7 @@ audio:not([controls]) { | |||
} | |||
[hidden], | |||
template { | |||
display: none; | |||
display: none !important; |
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.
Euch
|
||
// Trigger early service worker registration | ||
if ("serviceWorker" in navigator) { | ||
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.
We already register a service worker, right? Will this conflict with that?
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.
Nope, it doesn't, the second registration will just resolve instantly.
installPromptEvent.prompt(); | ||
} | ||
|
||
$(this).prop("hidden", true); |
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.
If the user removes it from the homescreen, that means this button won't be visible anymore, correct? Should we not remove it?
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.
Reloading the page will make it appear again, or depending on how the event gets called?
installPromptEvent
can be prompted only once anyway.
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.
Oh right, so it'll always re-show on refresh, of course. That feels less than ideal.
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.
Well it only shows whenever the browser feels like the user can be prompted (that's what Chrome devs suggest doing). It's on the settings page anyway.
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.
I meant the button on the settings page. Are we not hiding it when it's installed here, or have I misunderstood?
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.
The button only shows up when the browser triggers the event. I believe it won't trigger the event if you already have it on the homescreen (thus no button will show up).
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.
But the button is defined as just html...
<button type="button" class="btn" id="webapp-install-button" hidden>Add The Lounge to Home screen</button>
Does chrome find this button and hide it itself?
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.
hidden
attribute
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.
I'm an idiot.
const excludedPathsFromCache = /^(?:socket\.io|storage|uploads|cdn-cgi)\//; | ||
|
||
self.addEventListener("install", function() { | ||
self.skipWaiting(); |
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.
Where does this come 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.
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.
Good stuff, thanks @xPaw and previous reviewers!
This adds offline "support". The Lounge itself won't connect or show messages, but at least it will render the loading screen and will keep trying to connect to the server.
This is also required for "add to home" in Chrome 68, as they changed the rules.
And finally this allows generating a proper PWA apk on Android, as Chrome considers it a proper PWA app now.
@SinZ163 for review