Simplify HMR handling (#5811)

* Simplify HMR handling * Try skip test to reveal other test result * Support virtual files * Fix head injection * Revert CI changes * Bring back normalizeFilename * Refactor * Add changeset
withastro · Jan 11, 2023 · ec09bb6 · ec09bb6
1 parent 52209ca
commit ec09bb6
Show file tree

Hide file tree

Showing 8 changed files with 46 additions and 108 deletions.
diff --git a/.changeset/fluffy-mirrors-swim.md b/.changeset/fluffy-mirrors-swim.md
@@ -0,0 +1,5 @@
+---
+'astro': patch
+---
+
+Simplify HMR handling
diff --git a/packages/astro/src/core/compile/compile.ts b/packages/astro/src/core/compile/compile.ts
@@ -12,7 +12,6 @@ export interface CompileProps {
 	astroConfig: AstroConfig;
 	viteConfig: ResolvedConfig;
 	filename: string;
-	id: string | undefined;
 	source: string;
 }
 
@@ -25,7 +24,6 @@ export async function compile({
 	astroConfig,
 	viteConfig,
 	filename,
-	id: moduleId,
 	source,
 }: CompileProps): Promise<CompileResult> {
 	const cssDeps = new Set<string>();
@@ -37,7 +35,7 @@ export async function compile({
 		// use `sourcemap: "both"` so that sourcemap is included in the code
 		// result passed to esbuild, but also available in the catch handler.
 		transformResult = await transform(source, {
-			moduleId,
+			moduleId: filename,
 			pathname: filename,
 			projectRoot: astroConfig.root.toString(),
 			site: astroConfig.site?.toString(),

diff --git a/packages/astro/src/core/create-vite.ts b/packages/astro/src/core/create-vite.ts
@@ -102,7 +102,7 @@ export async function createVite(
 			astroIntegrationsContainerPlugin({ settings, logging }),
 			astroScriptsPageSSRPlugin({ settings }),
 			astroHeadPropagationPlugin({ settings }),
-			astroScannerPlugin({ settings, logging }),
+			astroScannerPlugin({ settings }),
 			astroInjectEnvTsPlugin({ settings, logging, fs }),
 			astroContentVirtualModPlugin({ settings }),
 			astroContentServerPlugin({ fs, settings, logging, mode }),

diff --git a/packages/astro/src/vite-plugin-astro/compile.ts b/packages/astro/src/vite-plugin-astro/compile.ts
@@ -7,7 +7,6 @@ import { getFileInfo } from '../vite-plugin-utils/index.js';
 
 interface CachedFullCompilation {
 	compileProps: CompileProps;
-	rawId: string;
 	logging: LogOptions;
 }
 
@@ -27,7 +26,6 @@ const FRONTMATTER_PARSE_REGEXP = /^\-\-\-(.*)^\-\-\-/ms;
 
 export async function cachedFullCompilation({
 	compileProps,
-	rawId,
 	logging,
 }: CachedFullCompilation): Promise<FullCompileResult> {
 	let transformResult: CompileResult;
@@ -37,7 +35,7 @@ export async function cachedFullCompilation({
 		transformResult = await cachedCompilation(compileProps);
 		// Compile all TypeScript to JavaScript.
 		// Also, catches invalid JS/TS in the compiled output before returning.
-		esbuildResult = await transformWithEsbuild(transformResult.code, rawId, {
+		esbuildResult = await transformWithEsbuild(transformResult.code, compileProps.filename, {
 			loader: 'ts',
 			target: 'esnext',
 			sourcemap: 'external',
@@ -51,15 +49,18 @@ export async function cachedFullCompilation({
 	} catch (err: any) {
 		await enhanceCompileError({
 			err,
-			id: rawId,
+			id: compileProps.filename,
 			source: compileProps.source,
 			config: compileProps.astroConfig,
 			logging: logging,
 		});
 		throw err;
 	}
 
-	const { fileId: file, fileUrl: url } = getFileInfo(rawId, compileProps.astroConfig);
+	const { fileId: file, fileUrl: url } = getFileInfo(
+		compileProps.filename,
+		compileProps.astroConfig
+	);
 
 	let SUFFIX = '';
 	SUFFIX += `\nconst $$file = ${JSON.stringify(file)};\nconst $$url = ${JSON.stringify(
@@ -70,7 +71,7 @@ export async function cachedFullCompilation({
 	if (!compileProps.viteConfig.isProduction) {
 		let i = 0;
 		while (i < transformResult.scripts.length) {
-			SUFFIX += `import "${rawId}?astro&type=script&index=${i}&lang.ts";`;
+			SUFFIX += `import "${compileProps.filename}?astro&type=script&index=${i}&lang.ts";`;
 			i++;
 		}
 	}

diff --git a/packages/astro/src/vite-plugin-astro/index.ts b/packages/astro/src/vite-plugin-astro/index.ts
@@ -4,20 +4,13 @@ import type { AstroSettings } from '../@types/astro';
 import type { LogOptions } from '../core/logger/core.js';
 import type { PluginMetadata as AstroPluginMetadata } from './types';
 
-import slash from 'slash';
-import { fileURLToPath } from 'url';
+import { normalizePath } from 'vite';
 import { cachedCompilation, CompileProps, getCachedCompileResult } from '../core/compile/index.js';
-import {
-	isRelativePath,
-	prependForwardSlash,
-	removeLeadingForwardSlashWindows,
-	startsWithForwardSlash,
-} from '../core/path.js';
-import { viteID } from '../core/util.js';
-import { normalizeFilename } from '../vite-plugin-utils/index.js';
+import { isRelativePath } from '../core/path.js';
 import { cachedFullCompilation } from './compile.js';
 import { handleHotUpdate } from './hmr.js';
-import { parseAstroRequest, ParsedRequestResult } from './query.js';
+import { parseAstroRequest } from './query.js';
+import { normalizeFilename } from '../vite-plugin-utils/index.js';
 export { getAstroMetadata } from './metadata.js';
 export type { AstroPluginMetadata };
 
@@ -27,85 +20,28 @@ interface AstroPluginOptions {
 }
 
 /** Transform .astro files for Vite */
-export default function astro({ settings, logging }: AstroPluginOptions): vite.Plugin {
+export default function astro({ settings, logging }: AstroPluginOptions): vite.Plugin[] {
 	const { config } = settings;
 	let resolvedConfig: vite.ResolvedConfig;
 
 	// Variables for determining if an id starts with /src...
 	const srcRootWeb = config.srcDir.pathname.slice(config.root.pathname.length - 1);
 	const isBrowserPath = (path: string) => path.startsWith(srcRootWeb) && srcRootWeb !== '/';
-	const isFullFilePath = (path: string) =>
-		path.startsWith(prependForwardSlash(slash(fileURLToPath(config.root))));
-
-	function relativeToRoot(pathname: string) {
-		const arg = startsWithForwardSlash(pathname) ? '.' + pathname : pathname;
-		const url = new URL(arg, config.root);
-		return slash(fileURLToPath(url)) + url.search;
-	}
 
-	function resolveRelativeFromAstroParent(id: string, parsedFrom: ParsedRequestResult): string {
-		const filename = normalizeFilename(parsedFrom.filename, config);
-		const resolvedURL = new URL(id, `file://${filename}`);
-		const resolved = resolvedURL.pathname;
-		if (isBrowserPath(resolved)) {
-			return relativeToRoot(resolved + resolvedURL.search);
-		}
-		return slash(fileURLToPath(resolvedURL)) + resolvedURL.search;
-	}
-
-	return {
+	const prePlugin: vite.Plugin = {
 		name: 'astro:build',
 		enforce: 'pre', // run transforms before other plugins can
 		configResolved(_resolvedConfig) {
 			resolvedConfig = _resolvedConfig;
 		},
-		// note: don’t claim .astro files with resolveId() — it prevents Vite from transpiling the final JS (import.meta.glob, etc.)
-		async resolveId(id, from, opts) {
-			// If resolving from an astro subresource such as a hoisted script,
-			// we need to resolve relative paths ourselves.
-			if (from) {
-				const parsedFrom = parseAstroRequest(from);
-				const isAstroScript = parsedFrom.query.astro && parsedFrom.query.type === 'script';
-				if (isAstroScript && isRelativePath(id)) {
-					return this.resolve(resolveRelativeFromAstroParent(id, parsedFrom), from, {
-						custom: opts.custom,
-						skipSelf: true,
-					});
-				}
-			}
-
-			// serve sub-part requests (*?astro) as virtual modules
-			const { query } = parseAstroRequest(id);
-			if (query.astro) {
-				// TODO: Try to remove these custom resolve so HMR is more predictable.
-				// Convert /src/pages/index.astro?astro&type=style to /Users/name/
-				// Because this needs to be the id for the Vite CSS plugin to property resolve
-				// relative @imports.
-				if (query.type === 'style' && isBrowserPath(id)) {
-					return relativeToRoot(id);
-				}
-				// Strip `/@fs` from linked dependencies outside of root so we can normalize
-				// it in the condition below. This ensures that the style module shared the same is
-				// part of the same "file" as the main Astro module in the module graph.
-				// "file" refers to `moduleGraph.fileToModulesMap`.
-				if (query.type === 'style' && id.startsWith('/@fs')) {
-					id = removeLeadingForwardSlashWindows(id.slice(4));
-				}
-				// Convert file paths to ViteID, meaning on Windows it omits the leading slash
-				if (isFullFilePath(id)) {
-					return viteID(new URL('file://' + id));
-				}
-				return id;
-			}
-		},
 		async load(id, opts) {
 			const parsedId = parseAstroRequest(id);
 			const query = parsedId.query;
 			if (!query.astro) {
 				return null;
 			}
 			// For CSS / hoisted scripts, the main Astro module should already be cached
-			const filename = normalizeFilename(parsedId.filename, config);
+			const filename = normalizePath(normalizeFilename(parsedId.filename, config.root));
 			const compileResult = getCachedCompileResult(config, filename);
 			if (!compileResult) {
 				return null;
@@ -152,7 +88,7 @@ export default function astro({ settings, logging }: AstroPluginOptions): vite.P
 						if (src.startsWith('/') && !isBrowserPath(src)) {
 							const publicDir = config.publicDir.pathname.replace(/\/$/, '').split('/').pop() + '/';
 							throw new Error(
-								`\n\n<script src="${src}"> references an asset in the "${publicDir}" directory. Please add the "is:inline" directive to keep this asset from being bundled.\n\nFile: ${filename}`
+								`\n\n<script src="${src}"> references an asset in the "${publicDir}" directory. Please add the "is:inline" directive to keep this asset from being bundled.\n\nFile: ${id}`
 							);
 						}
 					}
@@ -196,20 +132,14 @@ export default function astro({ settings, logging }: AstroPluginOptions): vite.P
 				return;
 			}
 
-			const filename = normalizeFilename(parsedId.filename, config);
 			const compileProps: CompileProps = {
 				astroConfig: config,
 				viteConfig: resolvedConfig,
-				filename,
-				id,
+				filename: normalizePath(parsedId.filename),
 				source,
 			};
 
-			const transformResult = await cachedFullCompilation({
-				compileProps,
-				rawId: id,
-				logging,
-			});
+			const transformResult = await cachedFullCompilation({ compileProps, logging });
 
 			for (const dep of transformResult.cssDeps) {
 				this.addWatchFile(dep);
@@ -242,7 +172,6 @@ export default function astro({ settings, logging }: AstroPluginOptions): vite.P
 				astroConfig: config,
 				viteConfig: resolvedConfig,
 				filename: context.file,
-				id: context.modules[0]?.id ?? undefined,
 				source: await context.read(),
 			};
 			const compile = () => cachedCompilation(compileProps);
@@ -253,6 +182,19 @@ export default function astro({ settings, logging }: AstroPluginOptions): vite.P
 			});
 		},
 	};
+
+	const normalPlugin: vite.Plugin = {
+		name: 'astro:build:normal',
+		resolveId(id) {
+			// If Vite resolver can't resolve the Astro request, it's likely a virtual Astro file, fallback here instead
+			const parsedId = parseAstroRequest(id);
+			if (parsedId.query.astro) {
+				return id;
+			}
+		},
+	};
+
+	return [prePlugin, normalPlugin];
 }
 
 function appendSourceMap(content: string, map?: string) {

diff --git a/packages/astro/src/vite-plugin-scanner/index.ts b/packages/astro/src/vite-plugin-scanner/index.ts
@@ -1,26 +1,18 @@
-import { Plugin as VitePlugin } from 'vite';
+import { normalizePath, Plugin as VitePlugin } from 'vite';
 import { AstroSettings } from '../@types/astro.js';
-import type { LogOptions } from '../core/logger/core.js';
 import { isEndpoint, isPage } from '../core/util.js';
-import { normalizeFilename } from '../vite-plugin-utils/index.js';
 
 import { scan } from './scan.js';
 
-export default function astroScannerPlugin({
-	settings,
-	logging,
-}: {
-	settings: AstroSettings;
-	logging: LogOptions;
-}): VitePlugin {
+export default function astroScannerPlugin({ settings }: { settings: AstroSettings }): VitePlugin {
 	return {
 		name: 'astro:scanner',
 		enforce: 'post',
 
 		async transform(this, code, id, options) {
 			if (!options?.ssr) return;
 
-			const filename = normalizeFilename(id, settings.config);
+			const filename = normalizePath(id);
 			let fileURL: URL;
 			try {
 				fileURL = new URL(`file://${filename}`);

diff --git a/packages/astro/src/vite-plugin-scripts/page-ssr.ts b/packages/astro/src/vite-plugin-scripts/page-ssr.ts
@@ -1,8 +1,7 @@
 import MagicString from 'magic-string';
-import { Plugin as VitePlugin } from 'vite';
+import { normalizePath, Plugin as VitePlugin } from 'vite';
 import { AstroSettings } from '../@types/astro.js';
 import { isPage } from '../core/util.js';
-import { normalizeFilename } from '../vite-plugin-utils/index.js';
 import { PAGE_SSR_SCRIPT_ID } from './index.js';
 
 export default function astroScriptsPostPlugin({
@@ -19,7 +18,7 @@ export default function astroScriptsPostPlugin({
 			const hasInjectedScript = settings.scripts.some((s) => s.stage === 'page-ssr');
 			if (!hasInjectedScript) return;
 
-			const filename = normalizeFilename(id, settings.config);
+			const filename = normalizePath(id);
 			let fileURL: URL;
 			try {
 				fileURL = new URL(`file://${filename}`);

diff --git a/packages/astro/src/vite-plugin-utils/index.ts b/packages/astro/src/vite-plugin-utils/index.ts
@@ -1,3 +1,4 @@
+import { fileURLToPath } from 'url';
 import ancestor from 'common-ancestor-path';
 import type { AstroConfig } from '../@types/astro';
 import {
@@ -44,11 +45,11 @@ export function getFileInfo(id: string, config: AstroConfig) {
  *
  * as absolute file paths with forward slashes.
  */
-export function normalizeFilename(filename: string, config: AstroConfig) {
+export function normalizeFilename(filename: string, root: URL) {
 	if (filename.startsWith('/@fs')) {
 		filename = filename.slice('/@fs'.length);
-	} else if (filename.startsWith('/') && !ancestor(filename, config.root.pathname)) {
-		const url = new URL('.' + filename, config.root);
+	} else if (filename.startsWith('/') && !ancestor(filename, fileURLToPath(root))) {
+		const url = new URL('.' + filename, root);
 		filename = viteID(url);
 	}
 	return removeLeadingForwardSlashWindows(filename);