Skip to content

fix: htmlFallbackMiddleware for favicon#15301

Merged
patak-cat merged 4 commits intomainfrom
fix/html-fallback-for-favicon
Dec 12, 2023
Merged

fix: htmlFallbackMiddleware for favicon#15301
patak-cat merged 4 commits intomainfrom
fix/html-fallback-for-favicon

Conversation

@patak-cat
Copy link
Copy Markdown
Member

Description

A favicon.ico request is returning the index.html because it has */* in its header. It seems that checking against image/ should work.


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

@bolt-new-by-stackblitz
Copy link
Copy Markdown

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

ArnaudBarre
ArnaudBarre previously approved these changes Dec 10, 2023
Comment thread packages/vite/src/node/server/middlewares/htmlFallback.ts Outdated
Copy link
Copy Markdown
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

I think it was intentional that the html fallback also apply to images, especially when we removed the dot rule. Perhaps we could special case for favicon.ico only? cc @sapphi-red

@sapphi-red
Copy link
Copy Markdown
Member

Yeah, if */* is included, we have to treat the same with text/html. I think a special case for favicon.ico makes sense. We already have a similar check here.

const knownIgnoreList = new Set(['/', '/favicon.ico'])

@patak-cat
Copy link
Copy Markdown
Member Author

Thanks! Updated to directly avoid the default /favicon.ico request.

@patak-cat patak-cat added the p3-minor-bug An edge case that only affects very specific usage (priority) label Dec 12, 2023
@patak-cat patak-cat merged commit c902545 into main Dec 12, 2023
@patak-cat patak-cat deleted the fix/html-fallback-for-favicon branch December 12, 2023 21:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

p3-minor-bug An edge case that only affects very specific usage (priority)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants