Simplify HMR for circular imports and CSS (#9706)

withastro · Jan 17, 2024 · 1539e04 · 1539e04
1 parent 6c64b14
commit 1539e04
Show file tree

Hide file tree

Showing 12 changed files with 110 additions and 103 deletions.
diff --git a/.changeset/curvy-seas-explain.md b/.changeset/curvy-seas-explain.md
@@ -0,0 +1,5 @@
+---
+"@astrojs/mdx": patch
+---
+
+Removes redundant HMR handling code
diff --git a/.changeset/tidy-planets-whisper.md b/.changeset/tidy-planets-whisper.md
@@ -0,0 +1,5 @@
+---
+"astro": patch
+---
+
+Simplifies HMR handling, improves circular dependency invalidation, and fixes Astro styles invalidation
diff --git a/packages/astro/e2e/fixtures/tailwindcss/astro.config.mjs b/packages/astro/e2e/fixtures/tailwindcss/astro.config.mjs
@@ -7,6 +7,7 @@ export default defineConfig({
 	integrations: [
 		tailwind({
 			configFile: fileURLToPath(new URL('./tailwind.config.js', import.meta.url)),
+			applyBaseStyles: false
 		}),
 	],
 	devToolbar: {

diff --git a/packages/astro/e2e/fixtures/tailwindcss/src/components/Layout.astro b/packages/astro/e2e/fixtures/tailwindcss/src/components/Layout.astro
@@ -0,0 +1,16 @@
+<html lang="en">
+	<head>
+		<meta charset="UTF-8" />
+		<link rel="icon" type="image/x-icon" href="/favicon.ico" />
+		<title>Astro + TailwindCSS</title>
+	</head>
+	<body class="bg-dawn text-midnight">
+    <slot/>
+  </body>
+</html>
+
+<style is:global>
+  @tailwind base;
+
+  @tailwind utilities;
+</style>
diff --git a/packages/astro/e2e/fixtures/tailwindcss/src/pages/index.astro b/packages/astro/e2e/fixtures/tailwindcss/src/pages/index.astro
@@ -1,18 +1,11 @@
 ---
 // Component Imports
+import Layout from '../components/Layout.astro';
 import Button from '../components/Button.astro';
 import Complex from '../components/Complex.astro';
 ---
 
-<html lang="en">
-	<head>
-		<meta charset="UTF-8" />
-		<link rel="icon" type="image/x-icon" href="/favicon.ico" />
-		<title>Astro + TailwindCSS</title>
-	</head>
-
-	<body class="bg-dawn text-midnight">
-		<Button>I’m a Tailwind Button!</Button>
-		<Complex />
-	</body>
-</html>
+<Layout>
+	<Button>I’m a Tailwind Button!</Button>
+	<Complex />
+</Layout>
diff --git a/packages/astro/src/core/logger/vite.ts b/packages/astro/src/core/logger/vite.ts
@@ -6,7 +6,7 @@ import { isLogLevelEnabled, type Logger as AstroLogger } from './core.js';
 
 const PKG_PREFIX = fileURLToPath(new URL('../../../', import.meta.url));
 const E2E_PREFIX = fileURLToPath(new URL('../../../e2e', import.meta.url));
-export function isAstroSrcFile(id: string | null) {
+function isAstroSrcFile(id: string | null) {
 	return id?.startsWith(PKG_PREFIX) && !id.startsWith(E2E_PREFIX);
 }
 

diff --git a/packages/astro/src/vite-plugin-astro/compile.ts b/packages/astro/src/vite-plugin-astro/compile.ts
@@ -76,11 +76,6 @@ export async function cachedFullCompilation({
 		}
 	}
 
-	// Prefer live reload to HMR in `.astro` files
-	if (!compileProps.viteConfig.isProduction) {
-		SUFFIX += `\nif (import.meta.hot) { import.meta.hot.decline() }`;
-	}
-
 	return {
 		...transformResult,
 		code: esbuildResult.code + SUFFIX,

diff --git a/packages/astro/src/vite-plugin-astro/hmr.ts b/packages/astro/src/vite-plugin-astro/hmr.ts
@@ -1,22 +1,23 @@
-import type { HmrContext, ModuleNode } from 'vite';
+import path from 'node:path';
+import { appendForwardSlash } from '@astrojs/internal-helpers/path';
+import type { HmrContext } from 'vite';
 import type { AstroConfig } from '../@types/astro.js';
 import type { cachedCompilation } from '../core/compile/index.js';
 import { invalidateCompilation, isCached, type CompileResult } from '../core/compile/index.js';
 import type { Logger } from '../core/logger/core.js';
-import { isAstroSrcFile } from '../core/logger/vite.js';
-import { isAstroScript } from './query.js';
 
 export interface HandleHotUpdateOptions {
 	config: AstroConfig;
 	logger: Logger;
+	astroFileToCssAstroDeps: Map<string, Set<string>>;
 
 	compile: () => ReturnType<typeof cachedCompilation>;
 	source: string;
 }
 
 export async function handleHotUpdate(
 	ctx: HmrContext,
-	{ config, logger, compile, source }: HandleHotUpdateOptions
+	{ config, logger, astroFileToCssAstroDeps, compile, source }: HandleHotUpdateOptions
 ) {
 	let isStyleOnlyChange = false;
 	if (ctx.file.endsWith('.astro') && isCached(config, ctx.file)) {
@@ -34,75 +35,45 @@ export async function handleHotUpdate(
 		invalidateCompilation(config, ctx.file);
 	}
 
-	// Skip monorepo files to avoid console spam
-	if (isAstroSrcFile(ctx.file)) {
-		return;
-	}
-
-	// go through each of these modules importers and invalidate any .astro compilation
-	// that needs to be rerun.
-	const filtered = new Set<ModuleNode>(ctx.modules);
-	const files = new Set<string>();
-	for (const mod of ctx.modules) {
-		// Skip monorepo files to avoid console spam
-		if (isAstroSrcFile(mod.id ?? mod.file)) {
-			filtered.delete(mod);
-			continue;
-		}
-
-		if (mod.file && isCached(config, mod.file)) {
-			filtered.add(mod);
-			files.add(mod.file);
-		}
-
-		for (const imp of mod.importers) {
-			if (imp.file && isCached(config, imp.file)) {
-				filtered.add(imp);
-				files.add(imp.file);
-			}
-		}
-	}
-
-	// Invalidate happens as a separate step because a single .astro file
-	// produces multiple CSS modules and we want to return all of those.
-	for (const file of files) {
-		if (isStyleOnlyChange && file === ctx.file) continue;
-		invalidateCompilation(config, file);
-		// If `ctx.file` is depended by an .astro file, e.g. via `this.addWatchFile`,
-		// Vite doesn't trigger updating that .astro file by default. See:
-		// https://github.com/vitejs/vite/issues/3216
-		// For now, we trigger the change manually here.
-		if (file.endsWith('.astro')) {
-			ctx.server.moduleGraph.onFileChange(file);
-		}
-	}
-
-	// Bugfix: sometimes style URLs get normalized and end with `lang.css=`
-	// These will cause full reloads, so filter them out here
-	const mods = [...filtered].filter((m) => !m.url.endsWith('='));
-
-	// If only styles are changed, remove the component file from the update list
 	if (isStyleOnlyChange) {
 		logger.debug('watch', 'style-only change');
 		// Only return the Astro styles that have changed!
-		return mods.filter((mod) => mod.id?.includes('astro&type=style'));
+		return ctx.modules.filter((mod) => mod.id?.includes('astro&type=style'));
 	}
 
-	// Add hoisted scripts so these get invalidated
-	for (const mod of mods) {
-		for (const imp of mod.importedModules) {
-			if (imp.id && isAstroScript(imp.id)) {
-				mods.push(imp);
+	// Edge case handling usually caused by Tailwind creating circular dependencies
+	//
+	// TODO: we can also workaround this with better CSS dependency management for Astro files,
+	// so that changes within style tags are scoped to itself. But it'll take a bit of work.
+	// https://github.com/withastro/astro/issues/9370#issuecomment-1850160421
+	for (const [astroFile, cssAstroDeps] of astroFileToCssAstroDeps) {
+		// If the `astroFile` has a CSS dependency on `ctx.file`, there's a good chance this causes a
+		// circular dependency, which Vite doesn't issue a full page reload. Workaround it by forcing a
+		// full page reload ourselves. (Vite bug)
+		// https://github.com/vitejs/vite/pull/15585
+		if (cssAstroDeps.has(ctx.file)) {
+			// Mimic the HMR log as if this file is updated
+			logger.info('watch', getShortName(ctx.file, ctx.server.config.root));
+			// Invalidate the modules of `astroFile` explicitly as Vite may incorrectly soft-invalidate
+			// the parent if the parent actually imported `ctx.file`, but `this.addWatchFile` was also called
+			// on `ctx.file`. Vite should do a hard-invalidation instead. (Vite bug)
+			const parentModules = ctx.server.moduleGraph.getModulesByFile(astroFile);
+			if (parentModules) {
+				for (const mod of parentModules) {
+					ctx.server.moduleGraph.invalidateModule(mod);
+				}
 			}
+			ctx.server.ws.send({ type: 'full-reload', path: '*' });
 		}
 	}
-
-	return mods;
 }
 
 function isStyleOnlyChanged(oldResult: CompileResult, newResult: CompileResult) {
 	return (
 		normalizeCode(oldResult.code) === normalizeCode(newResult.code) &&
+		// If style tags are added/removed, we need to regenerate the main Astro file
+		// so that its CSS imports are also added/removed
+		oldResult.css.length === newResult.css.length &&
 		!isArrayEqual(oldResult.css, newResult.css)
 	);
 }
@@ -129,3 +100,7 @@ function isArrayEqual(a: any[], b: any[]) {
 	}
 	return true;
 }
+
+function getShortName(file: string, root: string): string {
+	return file.startsWith(appendForwardSlash(root)) ? path.posix.relative(root, file) : file;
+}
diff --git a/packages/astro/src/vite-plugin-astro/index.ts b/packages/astro/src/vite-plugin-astro/index.ts
@@ -9,6 +9,7 @@ import {
 	cachedCompilation,
 	getCachedCompileResult,
 	type CompileProps,
+	invalidateCompilation,
 } from '../core/compile/index.js';
 import { isRelativePath } from '../core/path.js';
 import { normalizeFilename } from '../vite-plugin-utils/index.js';
@@ -27,7 +28,13 @@ interface AstroPluginOptions {
 export default function astro({ settings, logger }: AstroPluginOptions): vite.Plugin[] {
 	const { config } = settings;
 	let resolvedConfig: vite.ResolvedConfig;
-	let server: vite.ViteDevServer;
+	let server: vite.ViteDevServer | undefined;
+	// Tailwind styles could register Astro files as dependencies of other Astro files,
+	// causing circular imports which trips Vite's HMR. This set is passed to `handleHotUpdate`
+	// to force a page reload when these dependency files are updated
+	// NOTE: We need to initialize a map here and in `buildStart` because our unit tests don't
+	// call `buildStart` (test bug)
+	let astroFileToCssAstroDeps = new Map<string, Set<string>>();
 
 	// Variables for determining if an id starts with /src...
 	const srcRootWeb = config.srcDir.pathname.slice(config.root.pathname.length - 1);
@@ -42,6 +49,9 @@ export default function astro({ settings, logger }: AstroPluginOptions): vite.Pl
 		configureServer(_server) {
 			server = _server;
 		},
+		buildStart() {
+			astroFileToCssAstroDeps = new Map();
+		},
 		async load(id, opts) {
 			const parsedId = parseAstroRequest(id);
 			const query = parsedId.query;
@@ -157,11 +167,39 @@ export default function astro({ settings, logger }: AstroPluginOptions): vite.Pl
 				source,
 			};
 
+			// We invalidate and then compile again as we know Vite will only call this `transform`
+			// when its cache is invalidated.
+			// TODO: Do the compilation directly and remove our cache so we rely on Vite only.
+			invalidateCompilation(config, compileProps.filename);
 			const transformResult = await cachedFullCompilation({ compileProps, logger });
 
+			// Register dependencies of this module
+			const astroDeps = new Set<string>();
 			for (const dep of transformResult.cssDeps) {
+				if (dep.endsWith('.astro')) {
+					astroDeps.add(dep);
+				}
 				this.addWatchFile(dep);
 			}
+			astroFileToCssAstroDeps.set(id, astroDeps);
+
+			// When a dependency from the styles are updated, the dep and Astro module will get invalidated.
+			// However, the Astro style virtual module is not invalidated because we didn't register that the virtual
+			// module has that dependency. We currently can't do that either because of a Vite bug.
+			// https://github.com/vitejs/vite/pull/15608
+			// Here we manually invalidate the virtual modules ourselves when we're compiling the Astro module.
+			// When that bug is resolved, we can add the dependencies to the virtual module directly and remove this.
+			if (server) {
+				const mods = server.moduleGraph.getModulesByFile(compileProps.filename);
+				if (mods) {
+					const seen = new Set(mods);
+					for (const mod of mods) {
+						if (mod.url.includes('?astro')) {
+							server.moduleGraph.invalidateModule(mod, seen);
+						}
+					}
+				}
+			}
 
 			const astroMetadata: AstroPluginMetadata['astro'] = {
 				clientOnlyComponents: transformResult.clientOnlyComponents,
@@ -200,6 +238,7 @@ export default function astro({ settings, logger }: AstroPluginOptions): vite.Pl
 			return handleHotUpdate(context, {
 				config,
 				logger,
+				astroFileToCssAstroDeps,
 				compile,
 				source,
 			});

diff --git a/packages/astro/src/vite-plugin-astro/query.ts b/packages/astro/src/vite-plugin-astro/query.ts
@@ -35,8 +35,3 @@ export function parseAstroRequest(id: string): ParsedRequestResult {
 		query,
 	};
 }
-
-export function isAstroScript(id: string): boolean {
-	const parsed = parseAstroRequest(id);
-	return parsed.query.type === 'script';
-}
diff --git a/packages/astro/test/units/vite-plugin-astro/compile.test.js b/packages/astro/test/units/vite-plugin-astro/compile.test.js
@@ -62,11 +62,6 @@ const name = 'world
 		expect(result).to.be.undefined;
 	});
 
-	it('injects hmr code', async () => {
-		const result = await compile(`<h1>Hello World</h1>`, '/src/components/index.astro');
-		expect(result.code).to.include('import.meta.hot');
-	});
-
 	it('has file and url exports for markdwon compat', async () => {
 		const result = await compile(`<h1>Hello World</h1>`, '/src/components/index.astro');
 		await init;

diff --git a/packages/integrations/mdx/src/index.ts b/packages/integrations/mdx/src/index.ts
@@ -40,14 +40,8 @@ export default function mdx(partialMdxOptions: Partial<MdxOptions> = {}): AstroI
 		name: '@astrojs/mdx',
 		hooks: {
 			'astro:config:setup': async (params) => {
-				const {
-					updateConfig,
-					config,
-					addPageExtension,
-					addContentEntryType,
-					command,
-					addRenderer,
-				} = params as SetupHookParams;
+				const { updateConfig, config, addPageExtension, addContentEntryType, addRenderer } =
+					params as SetupHookParams;
 
 				addRenderer(astroJSXRenderer);
 				addPageExtension('.mdx');
@@ -209,12 +203,6 @@ export default function mdx(partialMdxOptions: Partial<MdxOptions> = {}): AstroI
 									code += `\nContent[Symbol.for('astro.needsHeadRendering')] = !Boolean(frontmatter.layout);`;
 									code += `\nContent.moduleId = ${JSON.stringify(id)};`;
 
-									if (command === 'dev') {
-										// TODO: decline HMR updates until we have a stable approach
-										code += `\nif (import.meta.hot) {
-											import.meta.hot.decline();
-										}`;
-									}
 									return { code, map: null };
 								},
 							},