fix: don't mix Vite plugins when spawning temporary Vite server (#6368)

* fix: don't mix Vite plugins when spawning temporary Vite server * chore: include command to `createVite` options * chore: use `command` and exclude `preview` * chore: add test * fix(test): remove command check from apply fn * chore: add hint about filtering vite plugins and command * chore: apply suggestion Co-authored-by: Ben Holmes <hey@bholmes.dev> --------- Co-authored-by: Nate Moore <natemoo-re@users.noreply.github.com> Co-authored-by: Ben Holmes <hey@bholmes.dev>
withastro · Feb 27, 2023 · 02a7266 · 02a7266
1 parent 95164bf
commit 02a7266
Show file tree

Hide file tree

Showing 6 changed files with 130 additions and 16 deletions.
diff --git a/.changeset/green-wombats-pay.md b/.changeset/green-wombats-pay.md
@@ -0,0 +1,5 @@
+---
+'astro': patch
+---
+
+Fix regression that caused some stateful Vite plugins to assume they were running in `dev` mode during the `build` and vice versa.
diff --git a/packages/astro/src/core/build/index.ts b/packages/astro/src/core/build/index.ts
@@ -77,7 +77,7 @@ class AstroBuilder {
 					middlewareMode: true,
 				},
 			},
-			{ settings: this.settings, logging, mode: 'build' }
+			{ settings: this.settings, logging, mode: 'build', command: 'build' }
 		);
 		await runHookConfigDone({ settings: this.settings, logging });
 

diff --git a/packages/astro/src/core/create-vite.ts b/packages/astro/src/core/create-vite.ts
@@ -30,6 +30,8 @@ interface CreateViteOptions {
 	settings: AstroSettings;
 	logging: LogOptions;
 	mode: 'dev' | 'build' | string;
+	// will be undefined when using `sync`
+	command?: 'dev' | 'build';
 	fs?: typeof nodeFs;
 }
 
@@ -48,7 +50,7 @@ const ALWAYS_NOEXTERNAL = [
 /** Return a common starting point for all Vite actions */
 export async function createVite(
 	commandConfig: vite.InlineConfig,
-	{ settings, logging, mode, fs = nodeFs }: CreateViteOptions
+	{ settings, logging, mode, command, fs = nodeFs }: CreateViteOptions
 ): Promise<vite.InlineConfig> {
 	const astroPkgsConfig = await crawlFrameworkPkgs({
 		root: fileURLToPath(settings.config.root),
@@ -170,7 +172,38 @@ export async function createVite(
 	//   3. integration-provided vite config, via the `config:setup` hook
 	//   4. command vite config, passed as the argument to this function
 	let result = commonConfig;
-	result = vite.mergeConfig(result, settings.config.vite || {});
+	// PR #6238 Calls user integration `astro:config:setup` hooks when running `astro sync`.
+  // Without proper filtering, user integrations may run twice unexpectedly:
+	// - with `command` set to `build/dev` (src/core/build/index.ts L72)
+	// - and again in the `sync` module to generate `Content Collections` (src/core/sync/index.ts L36)
+	// We need to check if the command is `build` or `dev` before merging the user-provided vite config.
+	// We also need to filter out the plugins that are not meant to be applied to the current command:
+	// - If the command is `build`, we filter out the plugins that are meant to be applied for `serve`.
+	// - If the command is `dev`, we filter out the plugins that are meant to be applied for `build`.
+	if (command) {
+		let plugins = settings.config.vite?.plugins;
+		if (plugins) {
+			const { plugins: _, ...rest } = settings.config.vite
+			const applyToFilter = command === 'build' ? 'serve' : 'build'
+			const applyArgs = [{...settings.config.vite, mode}, { command, mode }]
+			// @ts-expect-error ignore TS2589: Type instantiation is excessively deep and possibly infinite. 
+			plugins = plugins.flat(Infinity).filter((p) => {
+				if (!p || p?.apply === applyToFilter) {
+					return false;
+				}
+
+				if (typeof p.apply === 'function') {
+					return p.apply(applyArgs[0], applyArgs[1])
+				}
+
+				return true;
+			});
+
+			result = vite.mergeConfig(result, { ...rest, plugins });
+		} else {
+			result = vite.mergeConfig(result, settings.config.vite || {});
+		}
+	}
 	result = vite.mergeConfig(result, commandConfig);
 	if (result.plugins) {
 		sortPlugins(result.plugins);

diff --git a/packages/astro/src/core/dev/container.ts b/packages/astro/src/core/dev/container.ts
@@ -92,7 +92,7 @@ export async function createContainer(params: CreateContainerParams = {}): Promi
 					: 'undefined',
 			},
 		},
-		{ settings, logging, mode: 'dev', fs }
+		{ settings, logging, mode: 'dev', command: 'dev', fs }
 	);
 	await runHookConfigDone({ settings, logging });
 	const viteServer = await vite.createServer(viteConfig);

diff --git a/packages/astro/test/static-build-vite-plugins.test.js b/packages/astro/test/static-build-vite-plugins.test.js
@@ -0,0 +1,76 @@
+import { expect } from 'chai';
+import { loadFixture } from './test-utils.js';
+
+describe('Static build: vite plugins included when required', () => {
+	/** @type {Map<string, boolean>} */
+	const pluginsCalled = new Map();
+	/** @type {Map<string, boolean>} */
+	const expectedPluginResult = new Map([
+		['prepare-no-apply-plugin', true],
+		['prepare-serve-plugin', false],
+		['prepare-apply-fn-plugin', true],
+		['prepare-dont-apply-fn-plugin', false],
+		['prepare-build-plugin', true],
+	]);
+	before(async () => {
+		/** @type {import('./test-utils').Fixture} */
+		const fixture = await loadFixture({
+			root: './fixtures/astro pages/',
+			integrations: [
+				{
+					name: '@astrojs/prepare-vite-plugins',
+					hooks: {
+						'astro:config:setup': ({ updateConfig }) => {
+							pluginsCalled.set('prepare-no-apply-plugin', false);
+							pluginsCalled.set('prepare-serve-plugin', false);
+							pluginsCalled.set('prepare-apply-fn-plugin', false);
+							pluginsCalled.set('prepare-dont-apply-fn-plugin', false);
+							pluginsCalled.set('prepare-build-plugin', false);
+							updateConfig({
+								vite: {
+									plugins: [{
+										name: 'prepare-no-apply-plugin',
+										configResolved: () => {
+											pluginsCalled.set('prepare-no-apply-plugin', true);
+										}
+									}, {
+										name: 'prepare-serve-plugin',
+										apply: 'serve',
+										configResolved: () => {
+											pluginsCalled.set('prepare-serve-plugin', true);
+										}
+									}, {
+										name: 'prepare-apply-fn-plugin',
+										apply: (_, { command }) => command === 'build',
+										configResolved: () => {
+											pluginsCalled.set('prepare-apply-fn-plugin', true);
+										}
+									}, {
+										name: 'prepare-dont-apply-fn-plugin',
+										apply: (_, { command }) => command === 'serve',
+										configResolved: () => {
+											pluginsCalled.set('prepare-dont-apply-fn-plugin', true);
+										}
+									}, {
+										name: 'prepare-build-plugin',
+										apply: 'build',
+										configResolved: () => {
+											pluginsCalled.set('prepare-build-plugin', true);
+										}
+									}]
+								}
+							})
+						},
+					},
+				},
+			],
+		});
+		await fixture.build();
+	});
+	it('Vite Plugins are included/excluded properly', async () => {
+		expect(pluginsCalled.size).to.equal(expectedPluginResult.size, 'Not all plugins were initialized');
+		Array.from(expectedPluginResult.entries()).forEach(([plugin, called]) =>
+			expect(pluginsCalled.get(plugin)).to.equal(called, `${plugin} was ${called ? 'not' : ''} called`)
+		);
+	});
+});
diff --git a/packages/webapi/mod.d.ts b/packages/webapi/mod.d.ts
@@ -1,13 +1,13 @@
 // organize-imports-ignore
-export { pathToPosix } from './lib/utils';
-export { alert, ByteLengthQueuingStrategy, cancelAnimationFrame, cancelIdleCallback, CanvasRenderingContext2D, CharacterData, clearTimeout, Comment, CountQueuingStrategy, CSSStyleSheet, CustomElementRegistry, CustomEvent, Document, DocumentFragment, DOMException, Element, Event, EventTarget, fetch, File, FormData, Headers, HTMLBodyElement, HTMLCanvasElement, HTMLDivElement, HTMLDocument, HTMLElement, HTMLHeadElement, HTMLHtmlElement, HTMLImageElement, HTMLSpanElement, HTMLStyleElement, HTMLTemplateElement, HTMLUnknownElement, Image, ImageData, IntersectionObserver, MediaQueryList, MutationObserver, Node, NodeFilter, NodeIterator, OffscreenCanvas, ReadableByteStreamController, ReadableStream, ReadableStreamBYOBReader, ReadableStreamBYOBRequest, ReadableStreamDefaultController, ReadableStreamDefaultReader, Request, requestAnimationFrame, requestIdleCallback, ResizeObserver, Response, setTimeout, ShadowRoot, structuredClone, StyleSheet, Text, TransformStream, TreeWalker, URLPattern, Window, WritableStream, WritableStreamDefaultController, WritableStreamDefaultWriter, } from './mod.js';
-export declare const polyfill: {
-    (target: any, options?: PolyfillOptions): any;
-    internals(target: any, name: string): any;
-};
-interface PolyfillOptions {
-    exclude?: string | string[];
-    override?: Record<string, {
-        (...args: any[]): any;
-    }>;
-}
+export { pathToPosix } from './lib/utils';
+export { alert, ByteLengthQueuingStrategy, cancelAnimationFrame, cancelIdleCallback, CanvasRenderingContext2D, CharacterData, clearTimeout, Comment, CountQueuingStrategy, CSSStyleSheet, CustomElementRegistry, CustomEvent, Document, DocumentFragment, DOMException, Element, Event, EventTarget, fetch, File, FormData, Headers, HTMLBodyElement, HTMLCanvasElement, HTMLDivElement, HTMLDocument, HTMLElement, HTMLHeadElement, HTMLHtmlElement, HTMLImageElement, HTMLSpanElement, HTMLStyleElement, HTMLTemplateElement, HTMLUnknownElement, Image, ImageData, IntersectionObserver, MediaQueryList, MutationObserver, Node, NodeFilter, NodeIterator, OffscreenCanvas, ReadableByteStreamController, ReadableStream, ReadableStreamBYOBReader, ReadableStreamBYOBRequest, ReadableStreamDefaultController, ReadableStreamDefaultReader, Request, requestAnimationFrame, requestIdleCallback, ResizeObserver, Response, setTimeout, ShadowRoot, structuredClone, StyleSheet, Text, TransformStream, TreeWalker, URLPattern, Window, WritableStream, WritableStreamDefaultController, WritableStreamDefaultWriter, } from './mod.js';
+export declare const polyfill: {
+    (target: any, options?: PolyfillOptions): any;
+    internals(target: any, name: string): any;
+};
+interface PolyfillOptions {
+    exclude?: string | string[];
+    override?: Record<string, {
+        (...args: any[]): any;
+    }>;
+}