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

feat!: update to chokidar v4 #18453

Merged
merged 11 commits into from
Oct 29, 2024
Merged

feat!: update to chokidar v4 #18453

merged 11 commits into from
Oct 29, 2024

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Oct 24, 2024

Description

close #18129

NOTE: I started trying to bundle the chokidar types instead of inlining it, but it didn't work out as easily because chokidar's types includes a lot of internal stuff, which causes bundling issues, type compat issues, and large bundle size. At the end it's better to keep inlining it.

Due to explorations above, I've tweaked our code to work a little differently which I'll make comments below explaining


It also looks like we can remove the anymatch types that was used by chokidar before. I'm thinking perhaps that can be a follow-up.

export type {
FSWatcher,
WatchOptions,
AwaitWriteFinishOptions,
Copy link
Member Author

Choose a reason for hiding this comment

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

I removed this export because initially it wasn't exported by chokidar. It feels kinda rare to use this and I don't see it publicly used anywhere on github. So I figured to remove this.

packages/vite/src/node/server/index.ts Show resolved Hide resolved
@bluwy
Copy link
Member Author

bluwy commented Oct 24, 2024

Of course it has to be windows 🥲 I did get that error locally while developing, but the added await page.goto(viteTestUrl) fixed it for me. Not sure why not for windows.

EDIT: Looks like it's not necessarily windows now.

@bluwy
Copy link
Member Author

bluwy commented Oct 25, 2024

I'm able to reproduce locally sometimes by updating the vitest.config.e2e.ts with:

  1. procese.env.CI = true
  2. test.minWorkers: 1 and test.maxWorkers: 1

And after adding some loggings, here's what I got:

stdout | playground/hmr/__tests__/hmr.spec.ts > should hmr when file is deleted and restored
edit child

watch change /Users/bjorn/Work/oss/vite/playground-temp/hmr/file-delete-restore/child.js

edit parent
delete child

watch change /Users/bjorn/Work/oss/vite/playground-temp/hmr/file-delete-restore/parent.js

add child
edit parent

watch change /Users/bjorn/Work/oss/vite/playground-temp/hmr/file-delete-restore/parent.js
watch change /Users/bjorn/Work/oss/vite/playground-temp/hmr/file-delete-restore/child.js

add child/parent and edit child/parent are logged from the test. watch change/add/delete are logged from the fs watcher hooks.

From what I can tell, chokidar (or node fs) is collapsing the delete child and add child into a single file change event rather than 2 delete and add event. Perhaps it's related to paulmillr/chokidar#1361

Comment on lines 109 to 113
// chokidar -> fsevents
'fsevents-handler.js': {
src: `require('fsevents')`,
replacement: `__require('fsevents')`,
},
Copy link
Member

Choose a reason for hiding this comment

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

📝 Note to reviewers: chokidar now has ESM files so we don't need to patch require.

@@ -153,7 +147,6 @@ const moduleRunnerConfig = defineConfig({
'module-runner': path.resolve(__dirname, 'src/module-runner/index.ts'),
},
external: [
'fsevents',
Copy link
Member

Choose a reason for hiding this comment

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

📝 Note to reviewers: chokidar no longer uses fsevents (uses fs.watch for macos as well)

packages/vite/src/node/watch.ts Outdated Show resolved Hide resolved
@@ -446,10 +446,7 @@ export async function _createServer(
resolvedOutDirs,
)
const resolvedWatchOptions = resolveChokidarOptions(
{
disableGlobbing: true,
Copy link
Member

Choose a reason for hiding this comment

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

packages/vite/src/node/watch.ts Outdated Show resolved Hide resolved
@bluwy
Copy link
Member Author

bluwy commented Oct 28, 2024

Adding a timeout seems to make it more stable, though a bit unfortunate to have it. What's described in #18453 (comment) was causing the issue, and a timeout after the initial delete needs to be long enough for the fs to detect a deletion.

@bluwy bluwy added breaking change p3-significant High priority enhancement (priority) labels Oct 28, 2024
@sapphi-red

This comment was marked as outdated.

@vite-ecosystem-ci

This comment was marked as outdated.

@bluwy
Copy link
Member Author

bluwy commented Oct 29, 2024

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

📝 Ran ecosystem CI on 3ceb738: Open

suite result latest scheduled
redwoodjs failure failure
remix failure failure
sveltekit failure failure
vike failure failure
vite-environment-examples failure success
vitest failure failure

analogjs, astro, histoire, ladle, laravel, marko, nuxt, previewjs, quasar, qwik, rakkas, storybook, unocss, vite-plugin-pwa, vite-plugin-react, vite-plugin-react-swc, vite-plugin-svelte, vite-plugin-vue, vite-setup-catalogue, vitepress, vuepress

sapphi-red
sapphi-red previously approved these changes Oct 29, 2024
patak-dev
patak-dev previously approved these changes Oct 29, 2024
@bluwy bluwy dismissed stale reviews from patak-dev and sapphi-red via d155a53 October 29, 2024 08:25
@bluwy bluwy merged commit 192d555 into main Oct 29, 2024
15 checks passed
@bluwy bluwy deleted the chokidar-v4 branch October 29, 2024 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change p3-significant High priority enhancement (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade to chokidar v4
4 participants