-
Notifications
You must be signed in to change notification settings - Fork 3.4k
feat(vite-dev-server): migrate to async fs operations for better perf… #32910
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
base: develop
Are you sure you want to change the base?
Conversation
…ormance - Replace fs.readFileSync with async fs.readFile - Add proper error handling with debug logs - Improve performance by avoiding event loop blocking - Remove TODO comment about async fs methods This follows Node.js best practices and improves DX with faster build times.
|
|
|
| } | ||
|
|
||
| // Initialize loader promise | ||
| loaderPromise = loadInitFile() |
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.
Bug: Unhandled Promise Rejection in Plugin Loader
Potential unhandled promise rejection. The loaderPromise is initialized immediately when the plugin is created, but if the file read fails and transformIndexHtml is never called (e.g., in certain Vite configurations or error scenarios), the rejected promise will never be awaited, causing an unhandled promise rejection warning in Node.js. The old synchronous code would fail immediately at plugin creation, providing clearer error feedback. Consider adding a .catch() handler to the promise initialization or deferring the file read until it's actually needed in transformIndexHtml.
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.
@lucgonp We have an AI bot that reviews our PRs - sometimes the feedback is helpful, sometimes irrelevant - if you can respond accordingly here please.
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.
@lucgonp There are some errors in linting. You should be able to see them locally running: nx run @cypress/vite-dev-server:lint
Currently these errors are displaying:
$ eslint --ext .js,.ts,.json, .
/root/cypress/npm/vite-dev-server/src/plugins/cypress.ts
54:7 error Expected blank line before this statement padding-line-between-statements
55:7 error Expected blank line before this statement padding-line-between-statements
✖ 2 problems (2 errors, 0 warnings)
2 errors and 0 warnings potentially fixable with the `--fix` option.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
…ormance
This follows Node.js best practices and improves DX with faster build times.
Additional details
Steps to test
How has the user experience changed?
PR Tasks
cypress-documentation?type definitions?Note
Switches
cypress.tsto non-blocking file reads, preloads the init script, and adds debug-backed error handling.npm/vite-dev-server/src/plugins/cypress.ts):fs.readFileSyncand directfs.promises.readFilewith a shared promisifiedreadFilehelper.INIT_FILEPATHvialoaderPromiseand inject its contents intransformIndexHtml.loadInitFile()with debug logs and explicit error propagation.index.htmlvia asyncreadFileand inject discovered<script>tags and the preloaded loader script.Written by Cursor Bugbot for commit 8103f1c. This will update automatically on new commits. Configure here.