Use img elements for testing resource existence rather than fetch
#78
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
whymarrh
force-pushed
the
fix-icon-exists-checks-but-better
branch
from
4cef2bf to
f353851
Compare
rekmarks
requested changes
Jul 21, 2020
There was a problem hiding this comment.
I think this makes a lot of sense!
To follow up on questions in #77, I'm not positive why the same-origin restriction was added in the first place, likely out of an (over?)abundance of caution.
The whole point of resourceExists is to only send an href to the extension that actually points to something we can use. We have a similar fallback mechanism in the extension for when these links inevitably go stale, but I don't see any harm in doing some basic validation in the page so as to avoid sending complete garbage to the extension/mobile.
All of this said, let's rename resourceExists to imageExists in light of its new, more specific use?
whymarrh
force-pushed
the
fix-icon-exists-checks-but-better
branch
from
f353851 to
5c09a72
Compare
The previous implementation of `resourceExists` was using the `same-origin` mode for fetch but that breaks when a site uses another domain for their icons— `static.example.com` or `images.example.com`, for example. There isn't a fetch mode that works here: [1] - `same-origin`: doesn't work for the reasons described - `cors`: there's no guarantee that the 2nd origin in question is correctly configured for CORS—a quick check on two sites confirmed this - `navigate`: isn't applicable here - `no-cors` hides the `status` and `statusText` so we can't use those With that, this loads the resources as images and tests whether that works. This will essentially be a `no-cors` request but with the status exposed through `onload`+`onerror`, which will suffice for our use case. [1]:https://fetch.spec.whatwg.org/#concept-request-mode
whymarrh
force-pushed
the
fix-icon-exists-checks-but-better
branch
from
5c09a72 to
00f2a39
Compare
Closed
Merged
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.
Closes #77
The previous implementation of
resourceExistswas using thesame-originmode for fetch but that breaks when a site uses another domain for their icons—static.example.com,assets.example.com, orimages.example.com, for example.There isn't a fetch mode that works here:[1]
same-origindoesn't work for the reasons describedcorsthere's no guarantee that the 2nd origin in question is correctly configured for CORS—a quick check on two sites confirmed thisnavigateisn't applicable hereno-corshides thestatusandstatusTextso we can't use thoseWith that, this PR loads the resources as images and tests whether that works. This will essentially be a
no-corsrequest but with the status exposed throughonload+onerror, which will suffice for our use case.