-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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(deps): update vite #5311
chore(deps): update vite #5311
Conversation
✅ Deploy Preview for fastidious-cascaron-4ded94 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Maybe flakiness increased or maybe it's just same as before. I retried three times and Windows CI hasn't go through yet. |
Yea, it increased. I have another PR open with this change and it's not merged because of the flakiness. We need to rewrite the waitForWatcher to wait for a specific file. The watcher emits ready event too soon. |
@sheremet-va Do you know where this change is coming from? I couldn't find any specific change on Vite, so maybe something from chokidar (bumped to 3.6.0 in vitejs/vite#15803)? |
Could be anything, I don't know why this started happening, but watcher was always unstable. |
Maybe we can pass down a ENV variable and wait for it to appear in |
Thanks for the pointer. I didn't know you were referring For vite-node tests, we can wait for entry files to be included |
We can use something like this |
Failing CI is indicating that https://github.com/vitest-dev/vitest/actions/runs/8135325279/job/22229683329#step:10:977 vitest/test/test-utils/index.ts Lines 191 to 196 in c3eb8de
So, I'm not sure if this flakiness can be fixed by waiting any specific file by It maybe simply some performance degradation which makes Windows vmThreads harder to pass? |
@@ -131,6 +131,31 @@ async function run(files: string[], options: CliOptions = {}) { | |||
process.on('uncaughtException', (err) => { | |||
console.error(c.red('[vite-node] Failed to execute file: \n'), err) | |||
}) | |||
|
|||
if (process.env.VITE_TEST_WATCHER_DEBUG) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add process.env.VITE_TEST_WATCHER_DEBUG: false
in rollup
config (under a condition like "production" build) so this is removed when bundling?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like I can use process.env.ROLLUP_WATCH
to remove this only on build 17aa695
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, this obviously breaks CI since it uses build... Let me think about a different way.
This error happens more on Mac OS, to be honest. For some reason, this line is not reported at all. |
While searching I noticed this one vitest/packages/vite-node/src/hmr/emitter.ts Lines 33 to 40 in dcc8dda
Also on Vitest side: vitest/packages/vitest/src/node/plugins/index.ts Lines 124 to 128 in dcc8dda
Maybe tweak this to see if it helps anything? |
@hi-ogawa do you think this is stable enough? |
Ubuntu and macos CI seems okay. I cannot really judge Windows behavior, so I can only hope it's stable enough. |
Description
Related #5123 (comment), #5193
I tweaked vite-node watch test cases with a quick workaround. I'll see how others go on CI.
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
pnpm-lock.yaml
unless you introduce a new test example.Tests
pnpm test:ci
.Documentation
pnpm run docs
command.Changesets
feat:
,fix:
,perf:
,docs:
, orchore:
.