From 950481307f09539762286a3de13fd0f8a44a5b10 Mon Sep 17 00:00:00 2001 From: userquin Date: Sat, 25 Feb 2023 01:20:42 +0100 Subject: [PATCH 1/7] fix: don't mix Vite plugins when spawning temporary Vite server --- .changeset/green-wombats-pay.md | 5 +++++ packages/astro/src/core/create-vite.ts | 23 ++++++++++++++++++++++- packages/webapi/mod.d.ts | 24 ++++++++++++------------ 3 files changed, 39 insertions(+), 13 deletions(-) create mode 100644 .changeset/green-wombats-pay.md diff --git a/.changeset/green-wombats-pay.md b/.changeset/green-wombats-pay.md new file mode 100644 index 000000000000..2171e590ce89 --- /dev/null +++ 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: also excluding any plugin in preview. diff --git a/packages/astro/src/core/create-vite.ts b/packages/astro/src/core/create-vite.ts index c25668af4a83..18dc42464e87 100644 --- a/packages/astro/src/core/create-vite.ts +++ b/packages/astro/src/core/create-vite.ts @@ -170,7 +170,28 @@ 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 || {}); + if (mode !== 'preview') { + let plugins = settings.config.vite?.plugins; + if (plugins) { + const { plugins: _, ...rest } = settings.config.vite + const apply = mode === 'build' ? 'serve' : 'build' + // @ts-ignore we know what are doing + plugins = plugins.flat(Infinity).filter((p) => { + if (p?.apply === apply) + return false; + + // TODO: check this + // if (typeof p.apply === 'function') + // return p.apply() + + 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/webapi/mod.d.ts b/packages/webapi/mod.d.ts index ecc90236488d..00be1f2746d3 100644 --- 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; -} \ No newline at end of file +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; +} \ No newline at end of file From 3cf4e4d61e8da13ef0a075c307a0964a2b9adc21 Mon Sep 17 00:00:00 2001 From: userquin Date: Sat, 25 Feb 2023 02:14:46 +0100 Subject: [PATCH 2/7] chore: include command to `createVite` options --- packages/astro/src/core/build/index.ts | 2 +- packages/astro/src/core/create-vite.ts | 19 ++++++++++++------- packages/astro/src/core/dev/container.ts | 2 +- 3 files changed, 14 insertions(+), 9 deletions(-) diff --git a/packages/astro/src/core/build/index.ts b/packages/astro/src/core/build/index.ts index 638d78d105b9..206bed6e417e 100644 --- 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 index 18dc42464e87..b064e9c44cc8 100644 --- 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' | 'preview'; 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 { const astroPkgsConfig = await crawlFrameworkPkgs({ root: fileURLToPath(settings.config.root), @@ -170,19 +172,22 @@ 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; - if (mode !== 'preview') { + // command will be undefined when running sync + if (command && command !== 'preview') { let plugins = settings.config.vite?.plugins; if (plugins) { const { plugins: _, ...rest } = settings.config.vite - const apply = mode === 'build' ? 'serve' : 'build' + const applyToFilter = mode === 'build' ? 'serve' : 'build' + const applyArgs = [{...settings.config.vite, mode}, { command, mode }] // @ts-ignore we know what are doing plugins = plugins.flat(Infinity).filter((p) => { - if (p?.apply === apply) + if (!p || p?.apply === applyToFilter) { return false; + } - // TODO: check this - // if (typeof p.apply === 'function') - // return p.apply() + if (typeof p.apply === 'function') { + return p.apply(applyArgs[0], applyArgs[1]) + } return true; }); diff --git a/packages/astro/src/core/dev/container.ts b/packages/astro/src/core/dev/container.ts index 5b2b04835758..22c489b2e5da 100644 --- 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); From 7d3d86a09dbc30a32972005bd3243e6e0668cba5 Mon Sep 17 00:00:00 2001 From: userquin Date: Sat, 25 Feb 2023 12:43:32 +0100 Subject: [PATCH 3/7] chore: use `command` and exclude `preview` --- .changeset/green-wombats-pay.md | 2 +- packages/astro/src/core/create-vite.ts | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/.changeset/green-wombats-pay.md b/.changeset/green-wombats-pay.md index 2171e590ce89..031a85e959f2 100644 --- a/.changeset/green-wombats-pay.md +++ b/.changeset/green-wombats-pay.md @@ -2,4 +2,4 @@ 'astro': patch --- -Fix regression that caused some stateful Vite plugins to assume they were running in `dev` mode during the `build` and vice versa: also excluding any plugin in preview. +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/create-vite.ts b/packages/astro/src/core/create-vite.ts index b064e9c44cc8..9364700bbc51 100644 --- a/packages/astro/src/core/create-vite.ts +++ b/packages/astro/src/core/create-vite.ts @@ -31,7 +31,7 @@ interface CreateViteOptions { logging: LogOptions; mode: 'dev' | 'build' | string; // will be undefined when using `sync` - command?: 'dev' | 'build' | 'preview'; + command?: 'dev' | 'build'; fs?: typeof nodeFs; } @@ -173,11 +173,11 @@ export async function createVite( // 4. command vite config, passed as the argument to this function let result = commonConfig; // command will be undefined when running sync - if (command && command !== 'preview') { + if (command) { let plugins = settings.config.vite?.plugins; if (plugins) { const { plugins: _, ...rest } = settings.config.vite - const applyToFilter = mode === 'build' ? 'serve' : 'build' + const applyToFilter = command === 'build' ? 'serve' : 'build' const applyArgs = [{...settings.config.vite, mode}, { command, mode }] // @ts-ignore we know what are doing plugins = plugins.flat(Infinity).filter((p) => { From a48511c78feabc1a7976b58a4a8b1ea41d46b696 Mon Sep 17 00:00:00 2001 From: userquin Date: Sat, 25 Feb 2023 13:50:36 +0100 Subject: [PATCH 4/7] chore: add test --- .../test/static-build-vite-plugins.test.js | 76 +++++++++++++++++++ 1 file changed, 76 insertions(+) create mode 100644 packages/astro/test/static-build-vite-plugins.test.js diff --git a/packages/astro/test/static-build-vite-plugins.test.js b/packages/astro/test/static-build-vite-plugins.test.js new file mode 100644 index 000000000000..d9b4433c335a --- /dev/null +++ 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} */ + const pluginsCalled = new Map(); + /** @type {Map} */ + 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 || command === 'build', + configResolved: () => { + pluginsCalled.set('prepare-apply-fn-plugin', true); + } + }, { + name: 'prepare-dont-apply-fn-plugin', + apply: (_, { command }) => 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`) + ); + }); +}); From 725f74964ef0317a2bda5743477aaae2f0d29a74 Mon Sep 17 00:00:00 2001 From: userquin Date: Sat, 25 Feb 2023 14:14:48 +0100 Subject: [PATCH 5/7] fix(test): remove command check from apply fn --- packages/astro/test/static-build-vite-plugins.test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/astro/test/static-build-vite-plugins.test.js b/packages/astro/test/static-build-vite-plugins.test.js index d9b4433c335a..10fe44957b33 100644 --- a/packages/astro/test/static-build-vite-plugins.test.js +++ b/packages/astro/test/static-build-vite-plugins.test.js @@ -41,13 +41,13 @@ describe('Static build: vite plugins included when required', () => { } }, { name: 'prepare-apply-fn-plugin', - apply: (_, { command }) => !command || command === 'build', + apply: (_, { command }) => command === 'build', configResolved: () => { pluginsCalled.set('prepare-apply-fn-plugin', true); } }, { name: 'prepare-dont-apply-fn-plugin', - apply: (_, { command }) => command && command === 'serve', + apply: (_, { command }) => command === 'serve', configResolved: () => { pluginsCalled.set('prepare-dont-apply-fn-plugin', true); } From 3e013f1acfb8d3afb6ec3a9db40e95dc85345cbf Mon Sep 17 00:00:00 2001 From: userquin Date: Mon, 27 Feb 2023 17:17:37 +0100 Subject: [PATCH 6/7] chore: add hint about filtering vite plugins and command --- packages/astro/src/core/create-vite.ts | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/packages/astro/src/core/create-vite.ts b/packages/astro/src/core/create-vite.ts index 9364700bbc51..dc42dadf5c8c 100644 --- a/packages/astro/src/core/create-vite.ts +++ b/packages/astro/src/core/create-vite.ts @@ -172,14 +172,21 @@ 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; - // command will be undefined when running sync + // PR #6238 includes `astro sync` command, which is not a Vite command with an additional vite server. + // AstroBuilder::setup: will call createVite twice: + // - 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-ignore we know what are doing + // @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; From 0da567b9218541e95335dbf414a3560dd102897a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joaqu=C3=ADn=20S=C3=A1nchez?= Date: Mon, 27 Feb 2023 21:37:16 +0100 Subject: [PATCH 7/7] chore: apply suggestion Co-authored-by: Ben Holmes --- packages/astro/src/core/create-vite.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/astro/src/core/create-vite.ts b/packages/astro/src/core/create-vite.ts index dc42dadf5c8c..478a7b5fd31d 100644 --- a/packages/astro/src/core/create-vite.ts +++ b/packages/astro/src/core/create-vite.ts @@ -172,8 +172,8 @@ 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; - // PR #6238 includes `astro sync` command, which is not a Vite command with an additional vite server. - // AstroBuilder::setup: will call createVite twice: + // 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.