Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 11 commits into from
Feb 27, 2023
5 changes: 5 additions & 0 deletions .changeset/green-wombats-pay.md
Original file line number Diff line number Diff line change
@@ -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.
2 changes: 1 addition & 1 deletion packages/astro/src/core/build/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 });

Expand Down
37 changes: 35 additions & 2 deletions packages/astro/src/core/create-vite.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand All @@ -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),
Expand Down Expand Up @@ -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) {
natemoo-re marked this conversation as resolved.
Show resolved Hide resolved
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 || {});
}
}
Copy link
Contributor Author

@userquin userquin Feb 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add else case without plugins?

result = vite.mergeConfig(result, commandConfig);
if (result.plugins) {
sortPlugins(result.plugins);
Expand Down
2 changes: 1 addition & 1 deletion packages/astro/src/core/dev/container.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
76 changes: 76 additions & 0 deletions packages/astro/test/static-build-vite-plugins.test.js
Original file line number Diff line number Diff line change
@@ -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`)
);
});
});
24 changes: 12 additions & 12 deletions packages/webapi/mod.d.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
// organize-imports-ignore
natemoo-re marked this conversation as resolved.
Show resolved Hide resolved
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;
}>;
}