Prevent locking up when encountering invalid CSS (#4675)

* Prevent locking up when encountering invalid CSS

* Add a changeset

* Move the unit test to the test folder

* ponyfill aggregateerror

* Use the exported type

* keep original errors

* fix

.changeset/eleven-goats-confess.md

@@ -0,0 +1,5 @@
+---
+'astro': patch
+---
+
+Prevent locking up when encountering invalid CSS

packages/astro/package.json

@@ -88,13 +88,14 @@
     "dev": "astro-scripts dev --prebuild \"src/runtime/server/astro-island.ts\" --prebuild \"src/runtime/client/{idle,load,media,only,visible}.ts\" \"src/**/*.ts\"",
     "postbuild": "astro-scripts copy \"src/**/*.astro\"",
     "benchmark": "node test/benchmark/dev.bench.js && node test/benchmark/build.bench.js",
-    "test": "mocha --exit --timeout 20000 --ignore **/lit-element.test.js && mocha --timeout 20000 **/lit-element.test.js",
+    "test:unit": "mocha --exit --timeout 2000 ./test/units/**/*.test.js",
+    "test": "pnpm run test:unit && mocha --exit --timeout 20000 --ignore **/lit-element.test.js && mocha --timeout 20000 **/lit-element.test.js",
     "test:match": "mocha --timeout 20000 -g",
     "test:e2e": "playwright test",
     "test:e2e:match": "playwright test -g"
   },
   "dependencies": {
-    "@astrojs/compiler": "^0.23.5",
+    "@astrojs/compiler": "^0.24.0",
     "@astrojs/language-server": "^0.23.0",
     "@astrojs/markdown-remark": "^1.1.1",
     "@astrojs/telemetry": "^1.0.0",

packages/astro/src/core/compile/compile.ts

@@ -1,14 +1,12 @@
 import type { TransformResult } from '@astrojs/compiler';
-import type { PluginContext, SourceMapInput } from 'rollup';
-import type { ViteDevServer } from 'vite';
-import type { AstroConfig } from '../@types/astro';
-import type { TransformStyleWithVite } from './styles';
+import type { AstroConfig } from '../../@types/astro';
+import type { TransformStyle } from './types';
 
 import { transform } from '@astrojs/compiler';
-import { fileURLToPath } from 'url';
-import { AstroErrorCodes } from '../core/errors.js';
-import { prependForwardSlash } from '../core/path.js';
-import { viteID } from '../core/util.js';
+import { AstroErrorCodes } from '../errors.js';
+import { prependForwardSlash } from '../path.js';
+import { viteID, AggregateError } from '../util.js';
+import { createStylePreprocessor } from './style.js';
 
 type CompilationCache = Map<string, CompileResult>;
 type CompileResult = TransformResult & {
@@ -23,20 +21,7 @@ export interface CompileProps {
 	filename: string;
 	moduleId: string;
 	source: string;
-	ssr: boolean;
-	transformStyleWithVite: TransformStyleWithVite;
-	viteDevServer?: ViteDevServer;
-	pluginContext: PluginContext;
-}
-
-function getNormalizedID(filename: string): string {
-	try {
-		const filenameURL = new URL(`file://${filename}`);
-		return fileURLToPath(filenameURL);
-	} catch (err) {
-		// Not a real file, so just use the provided filename as the normalized id
-		return filename;
-	}
+	transformStyle: TransformStyle;
 }
 
 async function compile({
@@ -44,19 +29,10 @@ async function compile({
 	filename,
 	moduleId,
 	source,
-	ssr,
-	transformStyleWithVite,
-	viteDevServer,
-	pluginContext,
+	transformStyle,
 }: CompileProps): Promise<CompileResult> {
-	const normalizedID = getNormalizedID(filename);
 	let cssDeps = new Set<string>();
-	let cssTransformError: Error | undefined;
-
-	// handleHotUpdate doesn't have `addWatchFile` used by transformStyleWithVite.
-	if (!pluginContext.addWatchFile) {
-		pluginContext.addWatchFile = () => {};
-	}
+	let cssTransformErrors: Error[] = [];
 
 	// Transform from `.astro` to valid `.ts`
 	// use `sourcemap: "both"` so that sourcemap is included in the code
@@ -69,44 +45,11 @@ async function compile({
 		sourcefile: filename,
 		sourcemap: 'both',
 		internalURL: `/@fs${prependForwardSlash(
-			viteID(new URL('../runtime/server/index.js', import.meta.url))
+			viteID(new URL('../../runtime/server/index.js', import.meta.url))
 		)}`,
 		// TODO: baseline flag
 		experimentalStaticExtraction: true,
-		preprocessStyle: async (value: string, attrs: Record<string, string>) => {
-			const lang = `.${attrs?.lang || 'css'}`.toLowerCase();
-
-			try {
-				const result = await transformStyleWithVite.call(pluginContext, {
-					id: normalizedID,
-					source: value,
-					lang,
-					ssr,
-					viteDevServer,
-				});
-
-				if (!result) return null as any; // TODO: add type in compiler to fix "any"
-
-				for (const dep of result.deps) {
-					cssDeps.add(dep);
-				}
-
-				let map: SourceMapInput | undefined;
-				if (result.map) {
-					if (typeof result.map === 'string') {
-						map = result.map;
-					} else if (result.map.mappings) {
-						map = result.map.toString();
-					}
-				}
-
-				return { code: result.code, map };
-			} catch (err) {
-				// save error to throw in plugin context
-				cssTransformError = err as any;
-				return null;
-			}
-		},
+		preprocessStyle: createStylePreprocessor(transformStyle, cssDeps, cssTransformErrors),
 	})
 		.catch((err) => {
 			// throw compiler errors here if encountered
@@ -114,13 +57,21 @@ async function compile({
 			throw err;
 		})
 		.then((result) => {
-			// throw CSS transform errors here if encountered
-			if (cssTransformError) {
-				(cssTransformError as any).code =
-					(cssTransformError as any).code || AstroErrorCodes.UnknownCompilerCSSError;
-				throw cssTransformError;
+			switch(cssTransformErrors.length) {
+				case 0: return result;
+				case 1: {
+					let error = cssTransformErrors[0];
+					if(!(error as any).code) {
+						(error as any).code = AstroErrorCodes.UnknownCompilerCSSError;
+					}
+					throw cssTransformErrors[0];
+				}
+				default: {
+					const aggregateError = new AggregateError(cssTransformErrors);
+					(aggregateError as any).code = AstroErrorCodes.UnknownCompilerCSSError;
+					throw aggregateError;
+				}
 			}
-			return result;
 		});
 
 	const compileResult: CompileResult = Object.create(transformResult, {

packages/astro/src/core/compile/index.ts

@@ -0,0 +1,13 @@
+export type {
+	CompileProps
+} from './compile';
+export type {
+	TransformStyle
+} from './types';
+
+export {
+	cachedCompilation,
+	invalidateCompilation,
+	isCached,
+	getCachedSource
+} from './compile.js';

packages/astro/src/core/compile/style.ts

@@ -0,0 +1,39 @@
+import type { TransformOptions } from '@astrojs/compiler';
+import type { TransformStyle } from './types';
+import type { SourceMapInput } from 'rollup';
+
+type PreprocessStyle = TransformOptions['preprocessStyle'];
+
+export function createStylePreprocessor(transformStyle: TransformStyle, cssDeps: Set<string>, errors: Error[]): PreprocessStyle {
+	const preprocessStyle: PreprocessStyle = async (value: string, attrs: Record<string, string>) => {
+		const lang = `.${attrs?.lang || 'css'}`.toLowerCase();
+
+		try {
+			const result = await transformStyle(value, lang);
+
+			if (!result) return null as any; // TODO: add type in compiler to fix "any"
+
+			for (const dep of result.deps) {
+				cssDeps.add(dep);
+			}
+
+			let map: SourceMapInput | undefined;
+			if (result.map) {
+				if (typeof result.map === 'string') {
+					map = result.map;
+				} else if (result.map.mappings) {
+					map = result.map.toString();
+				}
+			}
+
+			return { code: result.code, map };
+		} catch (err) {
+			errors.push(err as unknown as Error);
+			return {
+				error: err + ''
+			};
+		}
+	};
+
+	return preprocessStyle;
+};

packages/astro/src/core/compile/types.ts

@@ -0,0 +1,9 @@
+import type { SourceMap } from 'rollup';
+
+export type TransformStyleResult = null | { 
+	code: string;
+	map: SourceMap | null;
+	deps: Set<string>;
+}
+
+export type TransformStyle = (source: string, lang: string) => TransformStyleResult | Promise<TransformStyleResult>;

packages/astro/src/core/util.ts

@@ -226,3 +226,11 @@ export async function resolveIdToUrl(viteServer: ViteDevServer, id: string) {
 	}
 	return VALID_ID_PREFIX + result.id;
 }
+
+export const AggregateError = typeof globalThis.AggregateError !== 'undefined' ? globalThis.AggregateError : class extends Error {
+	errors: Array<any> = [];
+	constructor( errors: Iterable<any>, message?: string | undefined) {
+			super(message);
+			this.errors = Array.from(errors);
+		}
+}

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

@@ -4,7 +4,7 @@ import type { AstroConfig } from '../@types/astro';
 import type { LogOptions } from '../core/logger/core.js';
 import { info } from '../core/logger/core.js';
 import * as msg from '../core/messages.js';
-import { cachedCompilation, invalidateCompilation, isCached } from './compile.js';
+import { cachedCompilation, invalidateCompilation, isCached } from '../core/compile/index.js';
 import { isAstroScript } from './query.js';
 
 const PKG_PREFIX = new URL('../../', import.meta.url);

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

@@ -3,6 +3,7 @@ import type * as vite from 'vite';
 import type { AstroConfig } from '../@types/astro';
 import type { LogOptions } from '../core/logger/core.js';
 import type { PluginMetadata as AstroPluginMetadata } from './types';
+import type { ViteStyleTransformer } from '../vite-style-transform';
 
 import ancestor from 'common-ancestor-path';
 import esbuild from 'esbuild';
@@ -10,10 +11,14 @@ import slash from 'slash';
 import { fileURLToPath } from 'url';
 import { isRelativePath, startsWithForwardSlash } from '../core/path.js';
 import { getFileInfo } from '../vite-plugin-utils/index.js';
-import { cachedCompilation, CompileProps, getCachedSource } from './compile.js';
+import {
+	cachedCompilation,
+	CompileProps,
+	getCachedSource
+} from '../core/compile/index.js';
 import { handleHotUpdate } from './hmr.js';
 import { parseAstroRequest, ParsedRequestResult } from './query.js';
-import { createTransformStyleWithViteFn, TransformStyleWithVite } from './styles.js';
+import { createViteStyleTransformer, createTransformStyles } from '../vite-style-transform/index.js';
 
 const FRONTMATTER_PARSE_REGEXP = /^\-\-\-(.*)^\-\-\-/ms;
 interface AstroPluginOptions {
@@ -38,7 +43,7 @@ export default function astro({ config, logging }: AstroPluginOptions): vite.Plu
 	}
 
 	let resolvedConfig: vite.ResolvedConfig;
-	let transformStyleWithVite: TransformStyleWithVite;
+	let styleTransformer: ViteStyleTransformer;
 	let viteDevServer: vite.ViteDevServer | undefined;
 
 	// Variables for determining if an id starts with /src...
@@ -60,10 +65,11 @@ export default function astro({ config, logging }: AstroPluginOptions): vite.Plu
 		enforce: 'pre', // run transforms before other plugins can
 		configResolved(_resolvedConfig) {
 			resolvedConfig = _resolvedConfig;
-			transformStyleWithVite = createTransformStyleWithViteFn(_resolvedConfig);
+			styleTransformer = createViteStyleTransformer(_resolvedConfig);
 		},
 		configureServer(server) {
 			viteDevServer = server;
+			styleTransformer.viteDevServer = server;
 		},
 		// 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) {
@@ -117,10 +123,7 @@ export default function astro({ config, logging }: AstroPluginOptions): vite.Plu
 				filename,
 				moduleId: id,
 				source,
-				ssr: Boolean(opts?.ssr),
-				transformStyleWithVite,
-				viteDevServer,
-				pluginContext: this,
+				transformStyle: createTransformStyles(styleTransformer, filename, Boolean(opts?.ssr), this),
 			};
 
 			switch (query.type) {
@@ -216,10 +219,7 @@ export default function astro({ config, logging }: AstroPluginOptions): vite.Plu
 				filename,
 				moduleId: id,
 				source,
-				ssr: Boolean(opts?.ssr),
-				transformStyleWithVite,
-				viteDevServer,
-				pluginContext: this,
+				transformStyle: createTransformStyles(styleTransformer, filename, Boolean(opts?.ssr), this),
 			};
 
 			try {
@@ -352,10 +352,7 @@ ${source}
 				filename: context.file,
 				moduleId: context.file,
 				source: await context.read(),
-				ssr: true,
-				transformStyleWithVite,
-				viteDevServer,
-				pluginContext: this,
+				transformStyle: createTransformStyles(styleTransformer, context.file, true, this),
 			};
 			const compile = () => cachedCompilation(compileProps);
 			return handleHotUpdate.call(this, context, {

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

@@ -9,11 +9,8 @@ import type { AstroConfig } from '../@types/astro';
 import { pagesVirtualModuleId } from '../core/app/index.js';
 import { collectErrorMetadata } from '../core/errors.js';
 import type { LogOptions } from '../core/logger/core.js';
-import { cachedCompilation, CompileProps } from '../vite-plugin-astro/compile.js';
-import {
-	createTransformStyleWithViteFn,
-	TransformStyleWithVite,
-} from '../vite-plugin-astro/styles.js';
+import { cachedCompilation, CompileProps } from '../core/compile/index.js';
+import { ViteStyleTransformer, createViteStyleTransformer, createTransformStyles } from '../vite-style-transform/index.js';
 import type { PluginMetadata as AstroPluginMetadata } from '../vite-plugin-astro/types';
 import { getFileInfo } from '../vite-plugin-utils/index.js';
 
@@ -64,14 +61,17 @@ export default function markdown({ config, logging }: AstroPluginOptions): Plugi
 		return false;
 	}
 
-	let transformStyleWithVite: TransformStyleWithVite;
+	let styleTransformer: ViteStyleTransformer;
 	let viteDevServer: ViteDevServer | undefined;
 
 	return {
 		name: 'astro:markdown',
 		enforce: 'pre',
 		configResolved(_resolvedConfig) {
-			transformStyleWithVite = createTransformStyleWithViteFn(_resolvedConfig);
+			styleTransformer = createViteStyleTransformer(_resolvedConfig);
+		},
+		configureServer(server) {
+			styleTransformer.viteDevServer = server;
 		},
 		async resolveId(id, importer, options) {
 			// Resolve any .md files with the `?content` cache buster. This should only come from
@@ -208,10 +208,7 @@ ${setup}`.trim();
 					filename,
 					moduleId: id,
 					source: astroResult,
-					ssr: Boolean(opts?.ssr),
-					transformStyleWithVite,
-					viteDevServer,
-					pluginContext: this,
+					transformStyle: createTransformStyles(styleTransformer, filename, Boolean(opts?.ssr), this),
 				};
 
 				let transformResult = await cachedCompilation(compileProps);

packages/astro/src/vite-style-transform/index.ts

@@ -0,0 +1,7 @@
+export type {
+	ViteStyleTransformer
+} from './style-transform';
+export {
+  createViteStyleTransformer,
+	createTransformStyles
+} from './style-transform.js';

packages/astro/src/vite-style-transform/style-transform.ts

@@ -0,0 +1,49 @@
+import type { PluginContext } from 'rollup';
+import type { TransformStyle } from '../core/compile/index';
+import { TransformStyleWithVite, createTransformStyleWithViteFn } from './transform-with-vite.js';
+import { fileURLToPath } from 'url';
+
+import type * as vite from 'vite';
+
+export type ViteStyleTransformer = {
+	viteDevServer?: vite.ViteDevServer;
+	transformStyleWithVite: TransformStyleWithVite;
+}
+
+export function createViteStyleTransformer(viteConfig: vite.ResolvedConfig): ViteStyleTransformer {
+	return {
+		transformStyleWithVite: createTransformStyleWithViteFn(viteConfig),
+	};
+}
+
+function getNormalizedIDForPostCSS(filename: string): string {
+	try {
+		const filenameURL = new URL(`file://${filename}`);
+		return fileURLToPath(filenameURL);
+	} catch (err) {
+		// Not a real file, so just use the provided filename as the normalized id
+		return filename;
+	}
+}
+
+export function createTransformStyles(viteStyleTransformer: ViteStyleTransformer, filename: string, ssr: boolean, pluginContext: PluginContext): TransformStyle {
+	// handleHotUpdate doesn't have `addWatchFile` used by transformStyleWithVite.
+	// TODO, refactor, why is this happening *here* ?
+	if (!pluginContext.addWatchFile) {
+		pluginContext.addWatchFile = () => {};
+	}
+
+	const normalizedID = getNormalizedIDForPostCSS(filename);
+
+	return async function(styleSource, lang) {
+		const result = await viteStyleTransformer.transformStyleWithVite.call(pluginContext, {
+			id: normalizedID,
+			source: styleSource,
+			lang,
+			ssr,
+			viteDevServer: viteStyleTransformer.viteDevServer,
+		});
+
+		return result;
+	};
+}

packages/astro/test/units/compile/invalid-css.test.js

@@ -0,0 +1,43 @@
+
+import { expect } from 'chai';
+import { cachedCompilation } from '../../../dist/core/compile/index.js';
+import { AggregateError } from '../../../dist/core/util.js';
+
+describe('astro/src/core/compile', () => {
+	describe('Invalid CSS', () => {
+		it('throws an aggregate error with the errors', async () => {
+			let error;
+			try {
+				let r = await cachedCompilation({
+					config: /** @type {any} */({
+						root: '/'
+					}),
+					filename: '/src/pages/index.astro',
+					moduleId: '/src/pages/index.astro',
+					source: `
+	---
+	---
+	<style lang="scss">
+		article:global(:is(h1, h2, h3, h4, h5, h6):hover {
+			color: purple;
+		}
+	</style>
+	<style lang="scss">
+		article:is(h1, h2, h3, h4, h5, h6:hover {
+			color: purple;
+		}
+	</style>
+	`,
+					transformStyle(source, lang) {
+						throw new Error('Invalid css');
+					}
+				});
+			} catch(err) {
+				error = err;
+			}
+
+			expect(error).to.be.an.instanceOf(AggregateError);
+			expect(error.errors[0].message).to.contain('Invalid css');
+		});
+	});
+});

pnpm-lock.yaml

@@ -338,7 +338,7 @@ importers:
 
   packages/astro:
     specifiers:
-      '@astrojs/compiler': ^0.23.5
+      '@astrojs/compiler': ^0.24.0
       '@astrojs/language-server': ^0.23.0
       '@astrojs/markdown-remark': ^1.1.1
       '@astrojs/telemetry': ^1.0.0
@@ -421,7 +421,7 @@ importers:
       yargs-parser: ^21.0.1
       zod: ^3.17.3
     dependencies:
-      '@astrojs/compiler': 0.23.5
+      '@astrojs/compiler': 0.24.0
       '@astrojs/language-server': 0.23.3
       '@astrojs/markdown-remark': link:../markdown/remark
       '@astrojs/telemetry': link:../telemetry
@@ -3232,6 +3232,10 @@ packages:
     resolution: {integrity: sha512-vBMPy9ok4iLapSyCCT1qsZ9dK7LkVFl9mObtLEmWiec9myGHS9h2kQY2xzPeFNJiWXUf9O6tSyQpQTy5As/p3g==}
     dev: false
 
+  /@astrojs/compiler/0.24.0:
+    resolution: {integrity: sha512-xZ81C/oMfExdF18I1Tyd2BKKzBqO+qYYctSy4iCwH4UWSo/4Y8A8MAzV1hG67uuE7hFRourSl6H5KUbhyChv/A==}
+    dev: false
+
   /@astrojs/language-server/0.23.3:
     resolution: {integrity: sha512-ROoMKo37NZ76pE/A2xHfjDlgfsNnFmkhL4+Wifs0L855n73SUCbnXz7ZaQktIGAq2Te2TpSjAawiOx0q9L5qeg==}
     hasBin: true