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: stop considering parent URLs as public file #11145

Merged
merged 3 commits into from Dec 23, 2022

Conversation

brillout
Copy link
Contributor

@brillout brillout commented Dec 1, 2022

Description

Currently, when the URL starts with /.../node_modules/ (e.g. /../node_modules/some-library/src/someFile.js) then checkPublicFile() wrongfully considers the file to be a public file.

This bug happens because path.join(publicDir, '/../node_modules/some-library/src/someFile.js') resolves to the actual file someFile.js which makes checkPublicFile() wrongfully believe that it's a public file.

Additional context

This sometimes happens for frameworks built on top of vite-plugin-ssr.

Reproduction: https://github.com/brillout/vite-reprod-11145.

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 Commit 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.

benmccann
benmccann previously approved these changes Dec 2, 2022
packages/vite/src/node/plugins/asset.ts Outdated Show resolved Hide resolved
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
@bluwy
Copy link
Member

bluwy commented Dec 2, 2022

I think it'll be great to have tests that can reproduce this. I'm not sure how this would happen in Vite currently, and maybe it's better to fix the root problem.

@brillout
Copy link
Contributor Author

brillout commented Dec 3, 2022

I think it'll be great to have tests that can reproduce this. I'm not sure how this would happen in Vite currently, and maybe it's better to fix the root problem.

👍 Makes sense. I will try to reproduce this as a test.

@brillout
Copy link
Contributor Author

Just had a closer look.

I'm not sure how this would happen in Vite currently

It happens when you have this:

import.meta.glob('/../path/to/**/*.js')

Which is a valid pattern:

"import.meta.glob(['/../fixture-b/*.ts'])",

I think it'll be great to have tests that can reproduce this

I think it requires a full-fledged playground (because importAnalysisPlugin() requires a publicDir and a dev server). So I'm not sure if it's worth it to have an entire new playground just to test this edge case?

@bluwy
Copy link
Member

bluwy commented Dec 11, 2022

That's quite a strange pattern to support. I'd ping Anthony but he's on vacation now. I'm not sure if we want to correctly support it though, and using relative ../../ instead would be safer. 🤔

@brillout
Copy link
Contributor Author

That's quite a strange pattern to support. I'd ping Anthony but he's on vacation now. I'm not sure if we want to correctly support it though, and using relative ../../ instead would be safer. 🤔

Virtual modules cannot use ../../, so the only option is /../../. (Since /../../ is relative to config.root, whereas ../../ is relative to __dirname but __dirname doens't exist in virutal modules.)

Hydrogen introduced the practice of globbing inside node_modules/some-dependency/ which VPS also does.

When using pnpm, the file paths ends up being /../node_modules/some-library/some-file.js.

@patak-dev
Copy link
Member

It looks like these paths are indeed valid. Interesting that this is the only place where things break though. If vite-ecosystem-ci passes, this LGTM.

@patak-dev
Copy link
Member

/ecosystem-ci run

@patak-dev patak-dev added the p3-minor-bug 🔨 An edge case that only affects very specific usage (priority) label Dec 22, 2022
@vite-ecosystem-ci
Copy link

vite-ecosystem-ci bot commented Dec 22, 2022

📝 Ran ecosystem CI: Open

suite result
astro ✅ success
histoire ✅ success
iles ✅ success
ladle ✅ success
laravel ✅ success
marko ✅ success
nuxt-framework ✅ success
previewjs ✅ success
rakkas ✅ success
sveltekit ✅ success
vite-plugin-ssr ✅ success
vite-plugin-react ✅ success
vite-plugin-react-swc ✅ success
vite-plugin-svelte ✅ success
vite-plugin-vue ✅ success
vite-setup-catalogue ✅ success
vitepress ✅ success
vitest ✅ success
windicss ✅ success

@patak-dev patak-dev merged commit 568a014 into vitejs:main Dec 23, 2022
7 of 12 checks passed
futurGH pushed a commit to futurGH/vite that referenced this pull request Feb 26, 2023
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.

None yet

4 participants