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

chore: add \0 to virtual files id #11261

Merged
merged 3 commits into from Dec 9, 2022
Merged

Conversation

sapphi-red
Copy link
Member

Description

If the resolvedId doesn't start with \0, that parent directory will be watched by rollup.
This PR changes the id and resolvedId for virtual files to avoid that.

fixes #10096

Additional context


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.

@sapphi-red sapphi-red added p2-nice-to-have Not breaking anything but nice to have (priority) breaking change and removed breaking change labels Dec 8, 2022
Comment on lines 16 to +17
if (id === modulePreloadPolyfillId) {
return id
return resolvedModulePreloadPolyfillId
Copy link
Member Author

Choose a reason for hiding this comment

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

@patak-dev
Copy link
Member

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

vite-ecosystem-ci bot commented Dec 9, 2022

📝 Ran ecosystem CI: Open

suite result
astro ✅ success
histoire ❌ failure
iles ✅ success
ladle ✅ success
laravel ✅ success
marko ✅ success
nuxt-framework ❌ failure
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 ❌ failure
vitepress ✅ success
vitest ✅ success
windicss ✅ success

@patak-dev
Copy link
Member

@sapphi-red we may need to wait until the next minor for this one

@sapphi-red
Copy link
Member Author

@patak-dev I think it's slightly safer to include this in 4.0. But I'm ok with delaying this until 4.1.
I guess the ecosystem-ci was failing because this PR's base commit was a bit old.

@patak-dev
Copy link
Member

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

vite-ecosystem-ci bot commented Dec 9, 2022

📝 Ran ecosystem CI: Open

suite result
astro ✅ success
histoire ✅ success
iles ✅ success
ladle ✅ success
laravel ✅ success
marko ✅ success
nuxt-framework ❌ failure
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 02cdfa9 into vitejs:main Dec 9, 2022
@patak-dev
Copy link
Member

You're right, it was just the base commit

@sapphi-red sapphi-red deleted the chore/add-0-to-id branch December 9, 2022 08:19
futurGH pushed a commit to futurGH/vite that referenced this pull request Feb 26, 2023
@Bcos241 Bcos241 mentioned this pull request May 17, 2023
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rollup is watching C:\ in windows environment, if dynamic imports are used
3 participants