Update getSiteIcon logic #75
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This change updates the `getSiteIcon` logic in two main ways:
1. Use `img` elements for testing resource existence rather than `fetch`
The previous implementation of `resourceExists` was using the `same-origin`
mode for fetch but that would break when a site used another domain for
their icons, `static.example.com` or `images.example.com`, for example. There
isn't a fetch mode that works here. There's no guarantee that the 2nd origin
in question is correctly configured for CORS—so that's `cors` and
`same-origin` out. `navigate` isn't applicable here. and `no-cors` hides the
`status` and `statusText` so we can't confirm that. [1]
2. Handle multiple link elements with `rel=icon`
It is very possible and even recommended that there are multiple icons, and
if we're going to check that they actually exist, we should fallback to the
other images listed.
e.g.:
<link rel="icon" type="image/png" href="https://cdn.yourwebsite.com/favicon-16x16.png" sizes="16x16">
<link rel="icon" type="image/png" href="https://cdn.yourwebsite.com/favicon-32x32.png" sizes="32x32">
<link rel="icon" type="image/png" href="https://cdn.yourwebsite.com/favicon-96x96.png" sizes="96x96">
There is a subtle functionality change here in that the element
matching `head > link[rel="shortcut icon"]` will be checked first, but that
isn't important, nor should it be.
[1]:https://fetch.spec.whatwg.org/#concept-request-mode
whymarrh
force-pushed
the
fix-site-metadata
branch
from
9df4c15
to
3f35a1e
Compare
This was referenced Jul 21, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change updates the
getSiteIconlogic in two ways:Use
imgelements for testing resource existence rather thanfetchThe previous implementation of
resourceExistswas using thesame-originmode for fetch, but that would break when a site used another domain for their icons,static.example.comorimages.example.com, for example. There isn't a fetch mode that works here. There's no guarantee that the 2nd origin in question is correctly configured for CORS—so that'scorsandsame-originout.navigatedoesn't apply andno-corshides thestatusandstatusTextso we can't confirm that.[1]Handle multiple link elements with
rel=iconIt is very possible and even recommended that there are multiple icons, and if we're going to check that they actually exist, we should fallback to the other images listed.
e.g.:
There is a subtle functionality change here in that the element matching
head > link[rel="shortcut icon"]will be checked first, but that isn't important, nor should it be.