Support "add to home screen" on mobile browsers. #3155
Conversation
|
Finally got around to actually testing this and I can confirm that it actually works. |
shell/client/00-startup.js
Outdated
| @@ -14,6 +14,12 @@ | |||
| // See the License for the specific language governing permissions and | |||
| // limitations under the License. | |||
|
|
|||
| if ('serviceWorker' in navigator) { | |||
| window.addEventListener('load', function() { | |||
There was a problem hiding this comment.
@zenhack Your Tabs -> spaces commit didn't touch this file, so I assume this isn't resolved here.
shell/public/pwa-service-worker.js
Outdated
| @@ -0,0 +1,18 @@ | |||
|
|
|||
| self.addEventListener('install', function(event) { | |||
| const urlsToCache = [ | |||
There was a problem hiding this comment.
Tabs -> spaces in this whole file.
shell/public/pwa-service-worker.js
Outdated
| })) | ||
| self.addEventListener('fetch', function(event) { | ||
| event.respondWith( | ||
| caches.match(event.request).then(function(response) { |
There was a problem hiding this comment.
Given that we aren't actually caching anything, should we remove the caches.match check here? It seems like it'll just make things slower.
Does it work if you don't register a fetch listener at all? (I think it should then fall back to the default behavior of sending the request to the server...)
There was a problem hiding this comment.
Ack, I have another commit locally that actually does cache static assets, apparently I forgot to push... sorry about the slopyness on this one.
|
Fixed, thanks.
Quoting Jacob Weisz (2020-02-03 12:47:49)
… @ocdtrekkie commented on this pull request.
__________________________________________________________________
In [1]shell/client/00-startup.js:
> @@ -14,6 +14,12 @@
// See the License for the specific language governing permissions and
// limitations under the License.
+if ('serviceWorker' in navigator) {
+ window.addEventListener('load', function() {
***@***.*** Your Tabs -> spaces commit didn't touch this file, so I
assume this isn't resolved here.
--
You are receiving this because you were mentioned.
Reply to this email directly, [3]view it on GitHub, or [4]unsubscribe.
Verweise
1. #3155 (comment)
2. https://github.com/zenhack
3. #3155?email_source=notifications&email_token=AAGXYPUET5ADPXBFUKGEDV3RBBKELA5CNFSM4IUVCN6KYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCUA5JAQ#discussion_r374245801
4. https://github.com/notifications/unsubscribe-auth/AAGXYPSFVXMXL3NUE2VFTS3RBBKELANCNFSM4IUVCN6A
|
shell/public/pwa-service-worker.js
Outdated
| "icons/icons-adec2a339de139470e74b4c487321525.woff", | ||
| ] | ||
| event.waitUntil(caches.open('sandstorm-cache').then(function(cache) { | ||
| return cache.addAll(urlsToCache); |
There was a problem hiding this comment.
Do we need to keep the list in-sync somehow?
What happens when the Sandstorm server is updated and the files change? Will the installation keep using the old versions?
There was a problem hiding this comment.
Re: sw updates, I remember getting an impression from the docs that it would auto-update the service worker. But yeah, looking back this is a little half-baked. I'm just going rip out the caching stuff entirely, and we can just to the bare minimum to get add to home screen working. Presumably browser caches will do an okay job on their own.
With this patch, sandstorm now supports "add to home screen" on modern
mobile browsers:
https://developer.mozilla.org/en-US/docs/Web/Progressive_web_apps/Add_to_home_screen
Notes:
* The new file `/public/sandstorm-gradient-logo.png` is the same image
as the corresponding `.svg`; At least Chrome requires images larger
than the small `icon32.png` we have, and doesn't support `.svg`.
* The service worker doesn't do anything right now; it's just
there because some browsers demand it to allow add to home screen.
At some point we could use this to cache static assets.
|
Ok, killed the half-baked caching logic, squashed and rebased onto master. |
|
It may be worthwhile to revisit the caching question later as we talk further about shared/static assets going forward, including possibly static assets from apps. |
|
Yeah, let's open a separate ticket to track that once this is merged. |
|
Oops, I guess this conflicted with @zarvox's refactor. |
|
Merged master back in, but see my comment here: |
|
This seems unlikely it's related to this PR, but the test on this PR blew apart spectacularly: https://github.com/sandstorm-io/sandstorm/actions/runs/43545271 Unfortunately, while we know that tests fail and are ignoring that they do, the only way to see spectacular fails is to manually check the log. |
|
Can confirm this still works after fixing the merge conflicts. |
With this patch, sandstorm now supports "add to home screen" on modern
mobile browsers:
https://developer.mozilla.org/en-US/docs/Web/Progressive_web_apps/Add_to_home_screen
Notes:
/public/sandstorm-gradient-logo.pngis the same imageas the corresponding
.svg; At least Chrome requires images largerthan the small
icon32.pngwe have, and doesn't support.svg.there because some browsers demand it to allow add to home screen.
At some point we should use this to cache static assets.
IMPORTANT: I haven't actually had a chance to test this with a real
phone browser yet; I just moved and my sandstorm box isn't set up yet.
Chrome dev tools seems to be happy with it though.