Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion packages/astro/src/core/app/dev/pipeline.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import type {
RouteData,
SSRElement,
} from '../../../types/public/index.js';
import { getDevCSSModuleName } from '../../../vite-plugin-css/util.js';
import { type HeadElements, Pipeline, type TryRewriteResult } from '../../base-pipeline.js';
import { ASTRO_VERSION } from '../../constants.js';
import { createModuleScriptElement, createStylesheetElementSet } from '../../render/ssr-element.js';
Expand Down Expand Up @@ -92,7 +93,7 @@ export class DevPipeline extends Pipeline {
scripts.add({ props: {}, children });
}

const { css } = await import('virtual:astro:dev-css');
const { css } = await import(getDevCSSModuleName(routeData.component));

// Pass framework CSS in as style tags to be appended to the page.
for (const { id, url: src, content } of css) {
Expand Down
3 changes: 1 addition & 2 deletions packages/astro/src/core/build/plugins/plugin-manifest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,7 @@ async function buildManifest(

scripts.push({
type: 'external',
value: prefixAssetPath(src),
value: src,
});
}

Expand All @@ -238,7 +238,6 @@ async function buildManifest(
const styles = pageData.styles
.sort(cssOrder)
.map(({ sheet }) => sheet)
.map((s) => (s.type === 'external' ? { ...s, src: prefixAssetPath(s.src) } : s))
.reduce(mergeInlineCss, []);

routes.push({
Expand Down
1 change: 1 addition & 0 deletions packages/astro/src/core/build/static-build.ts
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,7 @@ async function buildEnvironments(opts: StaticBuildOptions, internals: BuildInter
entryFileNames: `${settings.config.build.assets}/[name].[hash].js`,
chunkFileNames: `${settings.config.build.assets}/[name].[hash].js`,
assetFileNames: `${settings.config.build.assets}/[name].[hash][extname]`,
...viteConfig.environments?.client?.build?.rollupOptions?.output,
Copy link
Member

Choose a reason for hiding this comment

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

What are we fixing/adding here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the script tests you asked about below, this allows people to customize the client-side output names.

},
},
},
Expand Down
3 changes: 2 additions & 1 deletion packages/astro/src/vite-plugin-app/pipeline.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import type {
} from '../types/public/index.js';
import { getComponentMetadata } from '../vite-plugin-astro-server/metadata.js';
import { createResolve } from '../vite-plugin-astro-server/resolve.js';
import { getDevCSSModuleName } from '../vite-plugin-css/util.js';
import { PAGE_SCRIPT_ID } from '../vite-plugin-scripts/index.js';

export class AstroServerPipeline extends Pipeline {
Expand Down Expand Up @@ -120,7 +121,7 @@ export class AstroServerPipeline extends Pipeline {
}
}

const { css } = await import('virtual:astro:dev-css');
const { css } = await loader.import(getDevCSSModuleName(routeData.component));
Copy link
Member

Choose a reason for hiding this comment

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

If possible, we shouldn't rely on the loader anymore. We actually want to move away from it eventually, because it goes in conflicts with the new architecture.

In fact, it's possible we won't need this pipeline anymore once we're able to provide all the features using DevApp

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure I understand, you can't import Vite code using the real dynamic import. It fails without this change.

Copy link
Member

Choose a reason for hiding this comment

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

That's the concerning part. Why does it work with Cloudflare, and not without it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because that code runs inside of the Vite runtime and this does not. So when you use regular dynamic import you are using Node.js module loader which of course can't resolve this. So we use loader to make it load inside the Vite environment.


// Pass framework CSS in as style tags to be appended to the page.
const links = new Set<SSRElement>();
Expand Down
67 changes: 35 additions & 32 deletions packages/astro/src/vite-plugin-css/index.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import type { Plugin, RunnableDevEnvironment } from 'vite';
import { wrapId } from '../core/util.js';
import type { ImportedDevStyle, RoutesList } from '../types/astro.js';
import type { RouteData } from '../types/public/index.js';
import { isBuildableCSSRequest } from '../vite-plugin-astro-server/util.js';

interface AstroVitePluginOptions {
Expand All @@ -11,36 +10,43 @@ interface AstroVitePluginOptions {

const MODULE_DEV_CSS = 'virtual:astro:dev-css';
const RESOLVED_MODULE_DEV_CSS = '\0' + MODULE_DEV_CSS;
const MODULE_DEV_CSS_PREFIX = 'virtual:astro:dev-css:';
const RESOLVED_MODULE_DEV_CSS_PREFIX = '\0' + MODULE_DEV_CSS_PREFIX;
const ASTRO_CSS_EXTENSION_POST_PATTERN = '@_@';

/**
* Extract the original component path from a masked virtual module name.
* Inverse function of getVirtualModulePageName().
*/
function getComponentFromVirtualModuleCssName(virtualModulePrefix: string, id: string): string {
return id.slice(virtualModulePrefix.length).replace(new RegExp(ASTRO_CSS_EXTENSION_POST_PATTERN, 'g'), '.');
}

/**
* This plugin tracks the CSS that should be applied by route.
*
* The virtual module should be used only during development
* The virtual module should be used only during development.
* Per-route virtual modules are created to avoid invalidation loops.
*
* @param routesList
*/
export function astroDevCssPlugin({ routesList, command }: AstroVitePluginOptions): Plugin {
let environment: undefined | RunnableDevEnvironment = undefined;
let cssMap = new Set<ImportedDevStyle>();
let currentRoute: RouteData | undefined = undefined;
let routeCssMap = new Map<string, Set<ImportedDevStyle>>();
return {
name: MODULE_DEV_CSS,

async configureServer(server) {
environment = server.environments.ssr as RunnableDevEnvironment;

server.middlewares.use(async (req, _res, next) => {
if (!req.url) return next();

currentRoute = routesList.routes.find((r) => req.url && r.pattern.test(req.url));
return next();
});
},

resolveId(id) {
if (id === MODULE_DEV_CSS) {
return RESOLVED_MODULE_DEV_CSS;
}
if (id.startsWith(MODULE_DEV_CSS_PREFIX)) {
return RESOLVED_MODULE_DEV_CSS_PREFIX + id.slice(MODULE_DEV_CSS_PREFIX.length);
}
},

async load(id) {
Expand All @@ -49,6 +55,13 @@ export function astroDevCssPlugin({ routesList, command }: AstroVitePluginOption
code: `export const css = new Set()`,
};
}
if (id.startsWith(RESOLVED_MODULE_DEV_CSS_PREFIX)) {
const componentPath = getComponentFromVirtualModuleCssName(RESOLVED_MODULE_DEV_CSS_PREFIX, id);
const cssSet = routeCssMap.get(componentPath) || new Set<ImportedDevStyle>();
return {
code: `export const css = new Set(${JSON.stringify(Array.from(cssSet.values()))})`,
};
}
},

async transform(code, id) {
Expand All @@ -58,34 +71,24 @@ export function astroDevCssPlugin({ routesList, command }: AstroVitePluginOption
const info = this.getModuleInfo(id);
if (!info) return;

if (id.startsWith('/') && currentRoute) {
if (id.startsWith('/')) {
const mod = environment?.moduleGraph.getModuleById(id);
if (mod) {
if (isBuildableCSSRequest(id)) {
cssMap.add({
if (mod && isBuildableCSSRequest(id)) {
// Find which routes use CSS that imports this file
for (const route of routesList.routes) {
if (!routeCssMap.has(route.component)) {
routeCssMap.set(route.component, new Set());
}
const cssSet = routeCssMap.get(route.component)!;
cssSet.add({
content: code,
id: wrapId(mod.id ?? mod.url),
url: wrapId(mod.url),
});
environment?.moduleGraph.invalidateModule(mod);
return;
}
return;
}
}

const hasAddedCss = cssMap.size > 0;
const mod = environment?.moduleGraph.getModuleById(RESOLVED_MODULE_DEV_CSS);
if (mod) {
environment?.moduleGraph.invalidateModule(mod);
}

if (id === RESOLVED_MODULE_DEV_CSS && hasAddedCss) {
const moduleCode = `export const css = new Set(${JSON.stringify(Array.from(cssMap.values()))})`;
// We need to clear the map, so the next time we render a new page
// we return the new CSS
cssMap.clear();
return moduleCode;
}
},
};
}
11 changes: 11 additions & 0 deletions packages/astro/src/vite-plugin-css/util.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import { getVirtualModulePageName } from '../vite-plugin-pages/util.js';

const MODULE_DEV_CSS_PREFIX = 'virtual:astro:dev-css:';

/**
* Get the virtual module name for a dev CSS import.
* Usage: `await loader.import(getDevCSSModuleName(routeData.component))`
*/
export function getDevCSSModuleName(componentPath: string): string {
return getVirtualModulePageName(MODULE_DEV_CSS_PREFIX, componentPath);
}
Comment on lines +1 to +11
Copy link
Member

Choose a reason for hiding this comment

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

I think this utility should be moved next to its opposite function. Plus, we're repeating the module prefix, while we should instead use one constant to reduce errors

60 changes: 39 additions & 21 deletions packages/astro/test/ssr-script.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -138,14 +138,20 @@ describe('External scripts in SSR', () => {
vite: {
build: {
assetsInlineLimit: 0,
rollupOptions: {
output: {
entryFileNames: 'assets/entry.[hash].mjs',
chunkFileNames: 'assets/chunks/chunk.[hash].mjs',
assetFileNames: 'assets/asset.[hash][extname]',
},
},
},
environments: {
client: {
build: {
rollupOptions: {
output: {
entryFileNames: 'assets/entry.[hash].mjs',
chunkFileNames: 'assets/chunks/chunk.[hash].mjs',
assetFileNames: 'assets/asset.[hash][extname]',
}
}
}
}
}
Comment on lines +142 to +154
Copy link
Member

Choose a reason for hiding this comment

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

The PR doesn't mention anything about this change. Is it related to the CSS? If so, how does it affect it? If not, what does this change do?

},
});
await fixture.build();
Expand All @@ -166,14 +172,20 @@ describe('External scripts in SSR', () => {
vite: {
build: {
assetsInlineLimit: 0,
rollupOptions: {
output: {
entryFileNames: 'assets/entry.[hash].mjs',
chunkFileNames: 'assets/chunks/chunk.[hash].mjs',
assetFileNames: 'assets/asset.[hash][extname]',
},
},
},
environments: {
client: {
build: {
rollupOptions: {
output: {
entryFileNames: 'assets/entry.[hash].mjs',
chunkFileNames: 'assets/chunks/chunk.[hash].mjs',
assetFileNames: 'assets/asset.[hash][extname]',
}
}
}
}
}
},
base: '/hello',
});
Expand All @@ -198,14 +210,20 @@ describe('External scripts in SSR', () => {
vite: {
build: {
assetsInlineLimit: 0,
rollupOptions: {
output: {
entryFileNames: 'assets/entry.[hash].mjs',
chunkFileNames: 'assets/chunks/chunk.[hash].mjs',
assetFileNames: 'assets/asset.[hash][extname]',
},
},
},
environments: {
client: {
build: {
rollupOptions: {
output: {
entryFileNames: 'assets/entry.[hash].mjs',
chunkFileNames: 'assets/chunks/chunk.[hash].mjs',
assetFileNames: 'assets/asset.[hash][extname]',
}
}
}
}
}
},
});
await fixture.build();
Expand Down
Loading