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

Use edge function to handle placeholder fallbacks #4708

Closed
wants to merge 2 commits into from

Conversation

steverep
Copy link
Member

@steverep steverep commented Sep 21, 2023

Proposed change

Fixes #4707 by implementing proposed solution in the issue. After testing, will follow up with PRs for frontend and docs to use new URL search parameter.
I used a deno devcontainer to develop the function. I can push that config if desired.

Type of change

  • Add a new logo or icon for a new core integration
  • Add a missing icon or logo for an existing core integration
  • Add a new logo or icon for a custom integration (custom component)
  • Replace an existing icon or logo with a higher quality version
  • Removing an icon or logo

Additional information

Checklist

  • The added/replaced image(s) are PNG
  • Icon image size is 256x256px (icon.png)
  • hDPI icon image size is 512x512px for ([email protected])
  • Logo image size has min 128px, but max 256px, on the shortest side (logo.png)
  • hDPI logo image size has min 256px, but max 512px, on the shortest side ([email protected])

@steverep
Copy link
Member Author

Everything works as expected in the deploy preview. Tomorrow I will add rules so that _/ URLs continue to work for existing HA versions.

- `https://brands.home-assistant.io/_/[domain]/[email protected]`
- `https://brands.home-assistant.io/_/[domain]/[email protected]`
```
https://brands.home-assistant.io/[domain]/{dark_}[icon|logo]{@2x}.png
Copy link
Member

Choose a reason for hiding this comment

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

Please revert this. This might work for a programmer, but lots of users don't understand this at all.

This is also completely unrelated to the change provided.

Copy link
Member Author

Choose a reason for hiding this comment

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

The relation was to be consistent when showing a URL with the search parameter below, without repeating a list of 8 nearly identical lines. Could just do something like .../[domain]/[image] instead?

@@ -0,0 +1,38 @@
import type { Context } from "https://edge.netlify.com";
Copy link
Member

Choose a reason for hiding this comment

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

We can't use edge functions. These will costs us a lot of money.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ugh... I just stupidly assumed the open source plan was unlimited invocations. Apparently not even the enterprise plan is unlimited.

We could potentially keep the invocations under the limit by caching the function responses (invocation only occurs when it runs per their docs). Search parameter would then be out though as they only cache based on path name. Theoretically, invocations would then = number of files x 2 x deploys per month.

Working within the config file constraints, the best alternative would be to switch the redirects to rewrites and lose the ability to vary the cache control header.

Copy link
Member

@frenck frenck Sep 22, 2023

Choose a reason for hiding this comment

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

Sorry to say, but this is being solved on the wrong end of the spectrum IMHO.

This is a frontend problem, solve it in the frontend (that said, I don't see a problem at all, so the "problem" in that sentence is overkill).

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a frontend problem, solve it in the frontend

The frontend cannot control responses from https://brands.home-assistant.io. A solution there means unnecessary extra code, repeating the functionality here, and, most notably, wasted network requests that reduce performance.

(that said, I don't see a problem at all, so the "problem" in that sentence is overkill).

I did my best to explain it in the issue. "Sorry to say", but you not seeing a problem doesn't mean it doesn't exist. If you'd like to see it, pull the frontend PR, test it, and look at the errors in the browser console.

@steverep
Copy link
Member Author

Given Netlify's invocation limits and billing on edge functions, closing in favor of #4722

@steverep steverep closed this Sep 25, 2023
@steverep steverep deleted the placeholder-edge-function branch September 25, 2023 18:43
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.

Placeholder redirects and CORS don't mix
2 participants