Rework Vite and Astro logger for HMR messages #9139

bluwy · 2023-11-20T10:50:57Z

Changes

This PR removes the Vite logLevel: 'warn' default (now info instead), and reuse Vite's logger so we get properly HMR messages for files that actually changed.

This also refactors the logging flow as a follow-up from #9105

Examples

framework-vue (dev). Updated files src/components/Counter.vue, src/pages/index.astro, astro.config.mjs, and then package.json, which triggered these logs:

Open screenshot

NOTE: there's a leading slash inconcistency for [watch] messages, seems to be intended in Vite. Leading slash is the URL, non-leading slash is for local files.

framework-vue (build):

Open screenshot

core-image-errors fixture, error is only logged once:

Open screenshot

Notes

This change helps unblock using Vite's bindCLIShortcut utility for us to implement CLI shortcuts easily. I think it's good that plugins can log infos by default too, e.g. vite-plugin-qrcode doesn't log anything today because we block info messages.

Testing

Tested the logging manually. And ran the tests locally.

Docs

The logging changes shouldn't need to be documented.

changeset-bot · 2023-11-20T10:51:01Z

🦋 Changeset detected

Latest commit: 14012c3

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions

This PR is blocked because it contains a minor changeset. A reviewer will merge this at the next release if approved.

packages/astro/src/core/create-vite.ts

packages/astro/src/core/logger/vite.ts

packages/astro/src/vite-plugin-astro/hmr.ts

natemoo-re

This all looks quite reasonable!

bluwy · 2023-11-27T14:55:45Z

packages/astro/src/vite-plugin-astro/index.ts

-			const isSkipLog =
-				/astro\.config\.[cm][jt]s$/.test(filename) ||
-				/(\/|\\)\.astro(\/|\\)/.test(filename) ||
-				isPkgFile(filename);
-			if (!isSkipLog && dedupeHotUpdateLogs(filename)) {
-				logger.info('watch', filename.replace(config.root.pathname, '/'));
-			}


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.

FredKSchott · 2023-11-30T00:18:01Z

packages/astro/src/core/logger/vite.ts

+			const stripped = stripAnsi(msg);
+			let m;
+			// Rewrite HMR page reload message
+			if ((m = vitePageReloadMsg.exec(stripped))) {


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!

I've left a note in the description about the slash issue:

NOTE: there's a leading slash inconcistency for [watch] messages, seems to be intended in Vite. Leading slash is the URL, non-leading slash is for local files.

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 calls handleHotUpdate doesn't always trigger HMR, e.g. if you're editing files that are not part of your app.

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.

FredKSchott

I won't block this via GitHub, but I'm worried this is a step backwards on some of the improvements of #9105 and #9129.

I left some comments on the two specific spots! If there's a way that you can refactor & fix the original bug without impacting the logging behavior at all, then I'm +1 on merging!

I can also make time early tomorrow morning my time to talk through this, if it would helpful to not block you. LMK!

FredKSchott · 2023-11-30T06:30:28Z

@bluwy can you share some examples of errors that are logged, but not thrown as errors? My theory is that all Vite errors are also thrown, so the Vite error log is unnecessary. If that's not true and you can share some situations where that's not true, I'll feel better about moving from 'debug' to 'error' as the default. Otherwise I'd still argue to keep 'debug' as the default.

FredKSchott · 2023-11-30T06:36:27Z

Based on the new rebased branch I'm no longer -1. I'm still very nervous about so much string matching and the potential for those to drift in patch/minor releases, but I can stomach it if there's no better alternative.

It would be nice if this could encourage an effort to code errors and/or logs in Vite, so that this would be less flakey without needing to rely on string matching.

bluwy · 2023-11-30T07:00:33Z

@bluwy can you share some examples of errors that are logged, but not thrown as errors? My theory is that all Vite errors are also thrown, so the Vite error log is unnecessary. If that's not true and you can share some situations where that's not true, I'll feel better about moving from 'debug' to 'error' as the default. Otherwise I'd still argue to keep 'debug' as the default.

The full list can be found with a logger.error( search in the Vite repo. Some logger.error() calls aren't followed up with a throw since it's only logged. In brief, things like optimizer fails, http proxy, and websocket connection errors would be logged and it would be a bit dangerous to suppress them.

One example you can try is something like http://localhost:4321/@fs/Users/bjorn/.gitconfig (or similar) where you're accessing URLs outside your project:

Screenshot

Frankly it looks like a weird formatting issue. But besides that, I think we shouldn't hide that error.

Another example is with a @import "./non-existent"; in the Astro style tags:

Screenshot

Vite reports a better error than the default one.

FredKSchott

LGTM! Thanks for digging into the feedback @bluwy, I feel much better now (maybe even excited?) about the new approach, and the fact that we can always tweak this post 4.0 on a per-error basis.

Merging as beta

bluwy added 4 commits November 20, 2023 18:05

Initial cleanup

67e68b4

Rewrite HMR messages to Astro

7b25cac

Suppress Vite build message

ec7d8e0

Add changeset

e118127

github-actions bot added pkg: astro Related to the core `astro` package (scope) pr: docs A PR that includes documentation for review semver: minor Change triggers a `minor` release labels Nov 20, 2023

github-actions bot previously requested changes Nov 20, 2023

View reviewed changes

bluwy added 2 commits November 20, 2023 21:10

Suppress Astro errors in Vite logger

065c70b

Fix log level threshold check

7077426

bluwy commented Nov 20, 2023

View reviewed changes

packages/astro/src/core/create-vite.ts Show resolved Hide resolved

packages/astro/src/core/logger/vite.ts Show resolved Hide resolved

packages/astro/src/vite-plugin-astro/hmr.ts Show resolved Hide resolved

bluwy marked this pull request as ready for review November 20, 2023 13:34

bluwy requested a review from FredKSchott November 20, 2023 13:47

natemoo-re approved these changes Nov 20, 2023

View reviewed changes

Merge branch 'next' into rework-custom-logger

6e46fc0

bluwy mentioned this pull request Nov 22, 2023

Add CLI shortcuts #9159

Merged

bluwy commented Nov 27, 2023

View reviewed changes

Princesseuh approved these changes Nov 29, 2023

View reviewed changes

FredKSchott reviewed Nov 30, 2023

View reviewed changes

bluwy added 2 commits November 30, 2023 12:42

Merge branch 'next' into rework-custom-logger

c1e25fd

Suppress more errors

a8751f6

Base automatically changed from next to main November 30, 2023 15:03

FredKSchott approved these changes Nov 30, 2023

View reviewed changes

Merge branch 'main' into rework-custom-logger

14012c3

bluwy merged commit 459b264 into main Dec 1, 2023
13 checks passed

bluwy deleted the rework-custom-logger branch December 1, 2023 08:58

This was referenced Dec 1, 2023

[ci] release (beta) #9251

Merged

[ci] release #9307

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework Vite and Astro logger for HMR messages #9139

Rework Vite and Astro logger for HMR messages #9139

bluwy commented Nov 20, 2023 •

edited

Loading

changeset-bot bot commented Nov 20, 2023 •

edited

Loading

github-actions bot left a comment

natemoo-re left a comment

bluwy Nov 27, 2023

FredKSchott Nov 30, 2023

bluwy Nov 30, 2023

bluwy Nov 30, 2023

FredKSchott left a comment

FredKSchott commented Nov 30, 2023

FredKSchott commented Nov 30, 2023

bluwy commented Nov 30, 2023

FredKSchott left a comment

Rework Vite and Astro logger for HMR messages #9139

Rework Vite and Astro logger for HMR messages #9139

Conversation

bluwy commented Nov 20, 2023 • edited Loading

Changes

Examples

Notes

Testing

Docs

changeset-bot bot commented Nov 20, 2023 • edited Loading

🦋 Changeset detected

github-actions bot left a comment

Choose a reason for hiding this comment

natemoo-re left a comment

Choose a reason for hiding this comment

bluwy Nov 27, 2023

Choose a reason for hiding this comment

FredKSchott Nov 30, 2023

Choose a reason for hiding this comment

bluwy Nov 30, 2023

Choose a reason for hiding this comment

bluwy Nov 30, 2023

Choose a reason for hiding this comment

FredKSchott left a comment

Choose a reason for hiding this comment

FredKSchott commented Nov 30, 2023

FredKSchott commented Nov 30, 2023

bluwy commented Nov 30, 2023

FredKSchott left a comment

Choose a reason for hiding this comment

bluwy commented Nov 20, 2023 •

edited

Loading

changeset-bot bot commented Nov 20, 2023 •

edited

Loading