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

fix: ignore requests to /_favicon/ #819

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

fix: ignore requests to /_favicon/ #819

wants to merge 6 commits into from

Conversation

alajmo
Copy link

@alajmo alajmo commented Nov 2, 2023

Fixes #618

Copy link

changeset-bot bot commented Nov 2, 2023

🦋 Changeset detected

Latest commit: 4125df7

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@crxjs/vite-plugin Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

vercel bot commented Nov 2, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
vite-plugin-docs ⬜️ Ignored (Inspect) Visit Preview Mar 24, 2024 9:08am

Copy link
Contributor

@jacksteamdev jacksteamdev left a comment

Choose a reason for hiding this comment

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

Thanks for figuring this out! 💯 I requested a small change.

Comment on lines 37 to 44
if (url.href.includes('/_favicon/')) {
const response = await fetch(url.href)
return new Response(response.body, {
headers: {
'Content-Type': response.headers.get('Content-Type') ?? 'text/javascript',
},
})
}
Copy link
Contributor

@jacksteamdev jacksteamdev Jan 14, 2024

Choose a reason for hiding this comment

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

I imagine there are other use cases to not redirect to the dev server, so I think it's best if we handle all this in one place. What if we move url.href.includes('/_favicon/') up into line 23 and abstract it into a guard function?

Copy link
Author

Choose a reason for hiding this comment

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

Agree, I put them in a array const ignoreURLPaths = ['/_favicon/'], in case there are other paths to ignore as well (also perhaps allow users to add their own ignore paths). It's not as efficient as doing separate if statements as before (though V8 is probably optimizing these on its own), but as I mentioned in the future users might want to add other ignore paths?

@jacksteamdev jacksteamdev added vite-plugin Related to Vite plugin awaiting-feedback Further information is requested labels Jan 14, 2024
alajmo and others added 3 commits January 15, 2024 21:12
* fix: hmr error

* Create bright-peaches-wink.md

---------

Co-authored-by: Jack Steam <[email protected]>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-feedback Further information is requested vite-plugin Related to Vite plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dev client worker capturing /_favicon/ request in chrome manifest v3
3 participants