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: deep resolve side effects when glob does not contain / #11807

Merged
merged 2 commits into from
Jan 26, 2023

Conversation

mihkeleidast
Copy link
Contributor

Description

This will automatically prefix "simple globs" like *.css in sideEffects in package.json to **/*.css, so such files deep in the module would also be considered as side effects.

This is how other bundlers (e.g. webpack, rollup) behave as well, so this is sort of important for cross-bundler compatibility. We ran into this issue, as we updated vite to v4, but had not encountered such issue with any other bundler setup. As an example, here is the very similar code in rollup, the relevant PR and issue.

Additional context

I'd add tests, but did not immediately find any that relate to the loadPackageData function. Can you point me to the appropriate place?

Also, sorry for not filing an issue beforehand, but this change is so small, essentially copy-paste from rollup, so perhaps we can discuss directly in here.


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.

@patak-dev
Copy link
Member

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

vite-ecosystem-ci bot commented Jan 25, 2023

📝 Ran ecosystem CI: Open

suite result
astro ✅ success
histoire ❌ failure
iles ✅ success
ladle ✅ success
laravel ✅ success
marko ✅ success
nuxt-framework ✅ success
previewjs ❌ failure
qwik ❌ failure
rakkas ✅ success
sveltekit ✅ success
vite-plugin-ssr ✅ 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
windicss ✅ success

patak-dev
patak-dev previously approved these changes Jan 25, 2023
Copy link
Member

@patak-dev patak-dev left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. We don't currently have tests for sideEffects AFAICS. I think the proper place would be in the resolve playground. You can create local packages there to test these using our e2e infra.
We could also move forward and add the tests in another PR so we test this in the wild during the 4.1 beta

@patak-dev patak-dev added the p3-minor-bug An edge case that only affects very specific usage (priority) label Jan 25, 2023
@mihkeleidast
Copy link
Contributor Author

If there are no existing tests covering that particular logic, I'm not sure I can add tests for all of it from scratch. Was hoping there were some existing tests so I could only add coverage for the added logic. So perhaps indeed this can move forward as-is for now?

The ecosystem CI failures seem to be unrelated to this, right?

@patak-dev
Copy link
Member

Yes, the three ecosystem-ci are also failing on main and are being worked out with the maintainers of these projects.

We discussed this PR with the team today, let's merge it so we get testing during the beta and we can get this one released in 4.1 next week

@patak-dev patak-dev enabled auto-merge (squash) January 26, 2023 09:32
@patak-dev patak-dev merged commit f3a0c3b into vitejs:main Jan 26, 2023
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.

2 participants