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: run htmlFallbackMiddleware for no accept header requests #15025

Merged

Conversation

sapphi-red
Copy link
Member

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

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

📝 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

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.

@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

@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

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!

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
4 participants