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: fsp.rm removing files does not take effect #16032

Merged
merged 2 commits into from Apr 5, 2024

Conversation

btea
Copy link
Collaborator

@btea btea commented Feb 26, 2024

Description

When I execute pnpm dev --debug to start the project, a cache file will be generated in the .vite folder, but when I type q in the terminal to exit the project, the terminal will output information about removing the cache, but the corresponding cache files are not deleted.

I tried to use fs.rmSync to replace fsp.rm and the corresponding cache file could be successfully deleted.

Additional context

I am not sure whether it is related to the operating environment. The above problems were encountered and solved in my local Windows environment, and the corresponding node version is v18.16.1.


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.

Copy link

stackblitz bot commented Feb 26, 2024

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

@patak-dev
Copy link
Member

We're using the same fsp.rm pattern in other places in the code. Maybe we should have a new utility that uses fs.rmSync in Windows in all these places, and keep fsp.rm in the rest of the systems until we get a report. @sapphi-red did you experience this issue in Windows too?

@btea
Copy link
Collaborator Author

btea commented Feb 26, 2024

I also noticed that the old cache was being removed on every launch.

// Start with a fresh cache
debug?.(colors.green(`removing old cache dir ${depsCacheDir}`))
await fsp.rm(depsCacheDir, { recursive: true, force: true })

The value of depsCacheDir obtained here is xxx/node_modules/.vite/deps, but the folder name format of the generated cache file is xxx/node_modules/.vite/deps_temp_xxx, so the removal operation here seems useless. 🤔

@sapphi-red
Copy link
Member

I tested out with both Windows and WSL and it did happen. I guess it's not related to OS.

I think this happens because we call process.exit here.


process.exit doesn't wait for async tasks.

@btea
Copy link
Collaborator Author

btea commented Apr 1, 2024

Do you think we should differentiate the processing according to different systems or should we replace fsp.rm with fs.rmSync uniformly?

@bluwy
Copy link
Member

bluwy commented Apr 5, 2024

I can also repro this on macos. I think it's fine to update this single line to use rmSync. We can add a comment that it's needed for cleanup during exit.

@patak-dev patak-dev enabled auto-merge (squash) April 5, 2024 14:27
@patak-dev patak-dev merged commit b05c405 into vitejs:main Apr 5, 2024
10 checks passed
@btea btea deleted the fix/remove-cache-dir branch April 5, 2024 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants