Skip to content

Comments

fix: run htmlFallbackMiddleware for no accept header requests#15025

Merged
patak-dev merged 1 commit intovitejs:mainfrom
sapphi-red:fix/html-fallback-no-accept-header
Nov 17, 2023
Merged

fix: run htmlFallbackMiddleware for no accept header requests#15025
patak-dev merged 1 commit intovitejs:mainfrom
sapphi-red:fix/html-fallback-no-accept-header

Conversation

@sapphi-red
Copy link
Member

@sapphi-red sapphi-red commented Nov 17, 2023

Description

fixes #9520
fixes #15022

Additional context

https://github.com/bripkens/connect-history-api-fallback does this check.
https://github.com/bripkens/connect-history-api-fallback/blob/74564d61c2db06e302923a23bd459bb5dd91712b/lib/index.js#L11-L43

I also think this middleware should not check Accept: application/json. But I didn't change that because this part also comes from connect-history-api-fallback and didn't want to change the behavior much.

What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines, especially the Pull Request Guidelines.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Update the corresponding documentation if needed.
  • Ideally, include relevant tests that fail without this PR but pass with it.

@sapphi-red sapphi-red added the p3-minor-bug An edge case that only affects very specific usage (priority) label Nov 17, 2023
@sapphi-red
Copy link
Member Author

sapphi-red commented Nov 17, 2023

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

vite-ecosystem-ci bot commented Nov 17, 2023

📝 Ran ecosystem CI on 8e8a5bb: Open

suite result latest scheduled
analogjs ✅ success ✅ success
astro ✅ success ✅ success
histoire ❌ failure ❌ failure
ladle ✅ success ✅ success
laravel ❌ failure ❌ failure
marko ✅ success ✅ success
nuxt ✅ success ❌ failure
nx ❌ failure ❌ failure
previewjs ✅ success ✅ success
qwik ❌ failure ❌ failure
rakkas ✅ success ✅ success
sveltekit ❌ failure ❌ failure
unocss ✅ success ✅ success
vike ✅ success ✅ success
vite-plugin-pwa ✅ success ✅ success
vite-plugin-react ✅ success ✅ success
vite-plugin-react-pages ✅ success ✅ success
vite-plugin-react-swc ✅ success ✅ success
vite-plugin-svelte ✅ success ✅ success
vite-plugin-vue ✅ success ✅ success
vite-setup-catalogue ✅ success ✅ success
vitepress ✅ success ✅ success
vitest ✅ success ✅ success

bluwy
bluwy approved these changes Nov 17, 2023
View reviewed changes
Copy link
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'm ok with this if this is the more expected behaviour from users 👍

I'm also fine with removing the application/json check. From connect-history-api-fallback, I found that it's added because some people don't expect HTML files to be served for application/json;*/*, and some expect otherwise #9520 (comment). But I don't think this is an issue for us.

@patak-dev patak-dev merged commit b93dfe3 into vitejs:main Nov 17, 2023
This was referenced Nov 18, 2023
Merged
Merged
Merged
@sapphi-red sapphi-red deleted the fix/html-fallback-no-accept-header branch November 20, 2023 14:29
@sapphi-red sapphi-red mentioned this pull request Nov 21, 2023
Merged
9 tasks
@virtuoushub virtuoushub mentioned this pull request Nov 21, 2023
Closed
@mxxk
Copy link

mxxk commented Nov 22, 2023

Thanks for this fix! I stumbled upon it due to a case where Accept was present but contained application/json. In this instance, Vite was pinged with Accept: text/html, application/json, text/plain, */*, which was okay in Vite 4.5.0, but resulted in 404 in Vite 5.0.0 due to #14756.

For what it's worth, the Accept header was generated by cypress-io/github-action.

@sapphi-red
Copy link
Member Author

sapphi-red commented Nov 22, 2023

@mxxk That case should be fixed by #15059 which is released in v5.0.2. If it is still failing after upgrading the version to v5.0.2, please create a new issue with a minimal repro.

@mxxk
Copy link

mxxk commented Nov 22, 2023 •
edited
Loading

Whoops, I forgot to mention in #15025 (comment) that I verified that the problem is fixed in Vite 5.0.2. ✅

The original intent behind the comment was to capture what seemed like a different case from a missing Accept header (as stated in the two issues #9520 and #15022 fixed by this PR). However, as you pointed out, in my case, the fix came from #15059. Thank you for clarifying @sapphi-red!

@sapphi-red sapphi-red mentioned this pull request Jul 8, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@patak-dev patak-dev patak-dev approved these changes

@bluwy bluwy bluwy approved these changes

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

vite 5 preview return 404 on / if Accept Header is missing Vite's dev server returns 404 if the request does not contain an Accept header

4 participants

@sapphi-red @mxxk @patak-dev @bluwy