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

Cache brand images #17840

Merged
merged 6 commits into from
Oct 25, 2023
Merged

Cache brand images #17840

merged 6 commits into from
Oct 25, 2023

Conversation

steverep
Copy link
Member

@steverep steverep commented Sep 6, 2023

Proposed change

Needs home-assistant/brands#4652

Enable the service worker to cache any brand images used for speed and in case of a power/internet outage. Need to enable CORS so JS can read the response, otherwise the allocated cache size is heavily padded due to the opaque response.

Also...

  • Adds a couple missing referrerpolicy="no-referrer" attributes
  • Reorganize the service worker a bit to make it easier to read
  • Remove cacheable response plugin (StaleWhileRevalidate accepts opaque responses by default)

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (thank you!)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example configuration

Additional information

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

@github-actions github-actions bot added the Dependencies Pull requests that update a dependency file label Sep 6, 2023
@steverep
Copy link
Member Author

Brands PR is merged, so this should be good to go for 2023.10. I'll verify in a week when it should be fully propagated to Cloudflare.

@steverep steverep marked this pull request as draft September 15, 2023 13:42
bramkragten
bramkragten previously approved these changes Sep 19, 2023
@steverep
Copy link
Member Author

So I made a few fixes, namely:

  • Removed cache renaming for now as this will interfere with what is currently deleted on new SW install
  • Force CORS in SW fetch so responses from CSS background images are not opaque (this is a spec limitation at the moment - see [css-values] Define crossorigin, preload and async URL modifiers w3c/csswg-drafts#1603)
  • Removed the use of Object.assign for the state badge style which also caused an opaque response (probably because the object is supposed to be read only and this corrupts it somehow)

The remaining issues have to do with Netlify and the brands repo:

  • Because of the /_/ URLs, we're going to end up caching most images twice. We can fix this by switching to a query parameter instead.
  • Anything that currently gets redirected to a placeholder now fails with a CORS error. This is because Netlify doesn't return the access-control-allow-origin header with the 302. I'm looking into how to deal with this and I think a simple edge function is needed.

@bramkragten
Copy link
Member

Because of the /_/ URLs, we're going to end up caching most images twice. We can fix this by switching to a query parameter instead.

I think we should just create a webcomponent that handles the 404's and never use the fallback/placeholder url...

@steverep
Copy link
Member Author

I think we should just create a webcomponent that handles the 404's and never use the fallback/placeholder url...

I already realized there is no way not to cache everything twice without some sort of intervention like that, or alternatively having logic in the SW to handle it.

In any case, I've got a change almost done for the brands repo to deal with Netlify's shortcomings on CORS, so the double caching can be dealt with as a follow up. The images are small enough that it won't really blow up anything anyway.

@steverep
Copy link
Member Author

steverep commented Sep 22, 2023

Regarding using a custom element and no fallback URLs, it also just occurred to me that:

  • That only works for img elements. There's no way to catch a 404 for a CSS url(), except by intercepting with a custom handler in the service worker. And if that is relied on, the legacy build will be broken. The only potential solution I see would be to fetch it as a test, then put a valid URL in CSS or embed it, which seems like an awful lot of coding work just to double the browser work.
  • Media thumbnails coming from core that use brands URLs need to make sure to be sanitized without fallback before using (Update brands URLs to use fallback search parameter core#100701).

@bramkragten
Copy link
Member

bramkragten commented Sep 25, 2023

That only works for img elements. There's no way to catch a 404 for a CSS url(), except by intercepting with a custom handler in the service worker. And if that is relied on, the legacy build will be broken. The only potential solution I see would be to fetch it as a test, then put a valid URL in CSS or embed it, which seems like an awful lot of coding work just to double the browser work.

True, but I think we can work with just img elements and never use it in a css url()? If we make it an element and enforce always using that element I think it will not be that painful.

@steverep
Copy link
Member Author

True, but I think we can work with just img elements and never use it in a css url()? If we make it an element and enforce always using that element I think it will not be that painful.

Sure, but do you want that restriction going forward (on use of a server that HA controls nonetheless)? Aside from that, the bother for me is that it's still just an unnecessary extra network request. It would be mitigated a bit by caching the 404s too, but still.

Anyway, what we're discussing here only solves the problem when the service worker is working. Adjustments are needed when it isn't: home-assistant/brands#4718.

I opened a PR that will unblock this and fix that too: home-assistant/brands#4722

@steverep steverep force-pushed the cache-brand-images branch 3 times, most recently from 83b8968 to a3e2451 Compare October 12, 2023 17:14
@steverep steverep marked this pull request as ready for review October 25, 2023 03:54
@bramkragten bramkragten merged commit 01f51f3 into dev Oct 25, 2023
13 checks passed
@bramkragten bramkragten deleted the cache-brand-images branch October 25, 2023 07:28
@github-actions github-actions bot locked and limited conversation to collaborators Oct 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No internet - No icons in Integrations & Devices pages
2 participants