resoai / TileBoard

A simple yet highly configurable Dashboard for HomeAssistant
MIT License
1.63k stars 278 forks source link

Samsung phones: Progressive Web App lacks service worker #819

Closed akloeckner closed 2 years ago

akloeckner commented 2 years ago

I wondered why TileBoard wouldn't install as a PWA on my new samsung phone. It appears, we would need a (at least) dummy service worker installed.

Here's a list of requirements: https://superpwa.com/doc/test-pwa-samsung-internet-browser/

rchl commented 2 years ago

Are you sure this information is up-to-date? Chrome didn't seem to require a service worker and Samsung browser is based on Chromium. Of course they could have changed that condition but not sure why would they.

Can you check on Chrome browser if you can install it?

akloeckner commented 2 years ago

It appears, there is a difference between those two constructs:

That's what I found out by testing on my device. I cannot find any official info on this.

So, this seems to be an additional option in Samsung Internet. I noticed, because HA itself does provide the option to "install in app screen". I know that HA has a service worker. So, I think it is plausible, that this is the requirement we don't fulfil.

akloeckner commented 2 years ago

I did some more research and think, what I would like to see is ambient badging and installation as a WebAKP, which come with some requirements,

I also found out, that Chrome and Edge can install pages as apps even on Windows and Linux. I verified with both, that HA itself shows an installation symbol in the address bar while we don't. Maybe this will help with finding out, what is different...

grafik

grafik

I did try to fulfil all requirements in a test setup, and I cannot get it to show the badge. But I'm just taking stabs, actually, especially on the service worker bit...

akloeckner commented 2 years ago

Being able to test on a laptop really helped...

I now have a working version at https://github.com/akloeckner/TileBoard/tree/feat/webapk !

What did I change? I followed this advice: https://web.dev/install-criteria/

I could clean this up and start a PR. But I'd appreciate a brief hint on

rchl commented 2 years ago

I suppose you could create PR for it if you care about it.

Some quick comments after looking at your changes:

rchl commented 2 years ago

I could clean this up and start a PR. But I'd appreciate a brief hint on

* best practices to unregister the service worker.

* anything you notice already that could trip me off in the wrong direction.

I don't really have experience with service workers but I don't think we'd need to unregister the service worker. I don't see any specific time that would be good for doing that.

akloeckner commented 2 years ago

Thanks for your comments and I agreee with them. I'll give it a try later.

I did not understand the comment about the add-on and ingress, though. We could prevent the service worker from starting up in the add-on, if there is a way to detect that case. I don't have enough insight in the way add-ons work to judge if this differenciation is necessary. A quick search gave me at least one add-on with a service worker. As it's announced by frenck, I'd assume, that this is a sound add-on. I'll try and see, how they handle service workers in their add-on, and then I'l make a prposal PR.

rchl commented 2 years ago

Since our service worker would be a dummy and added just for the purpose of enabling app install, it shouldn't be needed when running in ingress as then we are running in a home assistant frontend that already has a service worker.

As for detecting ingress, we do something like that already in a way. For example there are these variables in the index file: https://github.com/resoai/TileBoard/blob/f4c537a56bf28179a4085125d5eb6f2b27fd239d/index.html.ejs#L33-L38 that are rewritten when running as an addon: https://github.com/resoai/TileBoard-addon/blob/f635379f2eb4b82999e31c2f77d56539832f7d38/addon/rootfs/etc/nginx/templates/direct.gtpl#L22-L26

So we could create a variable like IN_INGRESS and rewrite it to have true value when running in ingress.

akloeckner commented 2 years ago

:-) OK. Looks really hacky. But sure, that will do. I'll add that logic.