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(serve): check the 'Accept' header in the request before transform (fix #3502) #14121

Closed
wants to merge 7 commits into from

Conversation

oubenruing
Copy link

@oubenruing oubenruing commented Aug 16, 2023

Description

this PR fix #3502

Additional context

For a project started in Vite's serve mode:
a. If there is a folder named "foo" with a package.json inside it in the root directory.
b. If there is a JavaScript file named "bar.js" in the root directory.
When accessing http://localhost:3000/foo or http://localhost:3000/bar, it will result in an error.

Here's a very simple example for any project started with Vite's serve mode, when accessing http://localhost:3000/vite.
Assuming my project has a route /vite, this will not give me the expected result.

This is because Vite does not check the source of the request during transformation. Considering the address entered in the browser's address bar, there no need for transformation; it should directly return the page itself. When a user directly enters an address in the browser, the Accept header will always include the "text/html" field. Therefore, in this pull request, we use the "Accept" header with "text/html" to make this determination.

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.
  • Read the Pull Request Guidelines and follow the PR Title Convention.
  • 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).
  • Ideally, include relevant tests that fail without this PR but pass with it.

Review PR in StackBlitz Codeflow Submitted with StackBlitz Codeflow.

@stackblitz
Copy link

stackblitz bot commented Aug 16, 2023

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

@oubenruing oubenruing changed the title fix: Check the 'Accept' header in the request before transform (fix #3502) fix: check the 'Accept' header in the request before transform (fix #3502) Aug 16, 2023
@oubenruing oubenruing changed the title fix: check the 'Accept' header in the request before transform (fix #3502) fix(serve): check the 'Accept' header in the request before transform (fix #3502) Sep 1, 2023
@oubenruing oubenruing requested a review from haoqunjiang October 8, 2023 05:59
@haoqunjiang haoqunjiang added the p3-minor-bug An edge case that only affects very specific usage (priority) label Oct 8, 2023
@sapphi-red

This comment was marked as outdated.

@vite-ecosystem-ci

This comment was marked as outdated.

@sapphi-red
Copy link
Member

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

vite-ecosystem-ci bot commented Oct 9, 2023

📝 Ran ecosystem CI: Open

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

@oubenruing
Copy link
Author

/ecosystem-ci run

It appears that some tests have not passed. What should I do to address this?

@sapphi-red
Copy link
Member

qwik's fail is expected. But other fails seems to be caused by this PR.
We'll need to check why the tests are failing.
You can run the ecosystem-ci locally to take a deeper look: https://github.com/vitejs/vite-ecosystem-ci#via-shell-script

@sapphi-red
Copy link
Member

marko fails with main branch so I guess that fail isn't related to this PR as well.

@oubenruing
Copy link
Author

After using vite-plugin-html, this issue can be avoided, so close this PR.

@oubenruing oubenruing closed this Feb 23, 2024
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.

Wrong File Served When Route Conflicts with File in Project Root
3 participants