Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Rework Vite and Astro logger for HMR messages #9139
Rework Vite and Astro logger for HMR messages #9139
Changes from all commits
67e68b4
7b25cac
ec7d8e0
e118127
065c70b
7077426
6e46fc0
c1e25fd
a8751f6
14012c3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
The fact that some files are logged by Vite with a leading
/
and some are not is pretty odd and would confuse me as a user (especially on windows where I imagine one would have/
and the other would have\
). Does Vite consider this a bug?I also don't love matching strings like this with regex, since these error messages aren't usually covered by semver and can change unexpectedly across different versions.
I think I prefer the solution that keeps handling this ourselves in
handleHotUpdate
where we can control things ourselves, for those reasons. But I also could be missing some benefits!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.
I've left a note in the description about the slash issue:
I investigated that yesterday and it seems safer this way since you may sometime get
/@fs/some/fs/path
and it would not be safe to remove the initial slash.If we put back in
handleHotUpdate
, we won't be able to fix #8224 though. Since an update that callshandleHotUpdate
doesn't always trigger HMR, e.g. if you're editing files that are not part of your app.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.
I also didn't find a cleaner way besides the regex, though Vite hasn't changed those HMR messages across many majors now. It seems to be the easiest way to get the exact paths that would trigger HMR + whether the HMR will trigger a page reload or HMR update, which our logs don't differentiate now but we could.
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.
This part can be removed as Vite now logs the actual changed files instead. The dedupe should also not be needed now as we log things through the Vite logger.