Fix CSS chunking between multiple framework components (#6582)

* Fix CSS chunking between multiple framework components

* Better CSS dedupe handling

* Add more tests

* Update docs

.changeset/violet-islands-buy.md

@@ -0,0 +1,5 @@
+---
+'astro': patch
+---
+
+Fix CSS chunking and deduping between multiple Astro files and framework components

packages/astro/src/core/build/plugins/plugin-css.ts

@@ -55,7 +55,20 @@ export function rollupPluginAstroBuildCSS(options: PluginOptions): VitePlugin[]
 		{
 			name: 'astro:rollup-plugin-build-css',
 
+			transform(_, id) {
+				// In the SSR build, styles that are bundled are tracked in `internals.cssChunkModuleIds`.
+				// In the client build, if we're also bundling the same style, return an empty string to
+				// deduplicate the final CSS output.
+				if (options.target === 'client' && internals.cssChunkModuleIds.has(id)) {
+					return '';
+				}
+			},
+
 			outputOptions(outputOptions) {
+				// Skip in client builds as its module graph doesn't have reference to Astro pages
+				// to be able to chunk based on it's related top-level pages.
+				if (options.target === 'client') return;
+
 				const assetFileNames = outputOptions.assetFileNames;
 				const namingIncludesHash = assetFileNames?.toString().includes('[hash]');
 				const createNameForParentPages = namingIncludesHash
@@ -133,17 +146,6 @@ export function rollupPluginAstroBuildCSS(options: PluginOptions): VitePlugin[]
 										internals.cssChunkModuleIds.add(id);
 									}
 								}
-								// In the client build, we bail if the chunk is a duplicated CSS chunk tracked from
-								// above. We remove all the importedCss to prevent emitting the CSS asset.
-								if (options.target === 'client') {
-									if (Object.keys(c.modules).every((id) => internals.cssChunkModuleIds.has(id))) {
-										for (const importedCssImport of meta.importedCss) {
-											delete bundle[importedCssImport];
-											meta.importedCss.delete(importedCssImport);
-										}
-										return;
-									}
-								}
 
 								// For the client build, client:only styles need to be mapped
 								// over to their page. For this chunk, determine if it's a child of a

packages/astro/test/astro-client-only.test.js

@@ -28,8 +28,12 @@ describe('Client only components', () => {
 		const html = await fixture.readFile('/index.html');
 		const $ = cheerioLoad(html);
 
-		const href = $('link[rel=stylesheet]').attr('href');
-		const css = await fixture.readFile(href);
+		const stylesheets = await Promise.all(
+			$('link[rel=stylesheet]').map((_, el) => {
+				return fixture.readFile(el.attribs.href);
+			})
+		);
+		const css = stylesheets.join('');
 
 		expect(css).to.match(/yellowgreen/, 'Svelte styles are added');
 		expect(css).to.match(/Courier New/, 'Global styles are added');
@@ -87,8 +91,12 @@ describe('Client only components subpath', () => {
 		const html = await fixture.readFile('/index.html');
 		const $ = cheerioLoad(html);
 
-		const href = $('link[rel=stylesheet]').attr('href');
-		const css = await fixture.readFile(href.replace(/\/blog/, ''));
+		const stylesheets = await Promise.all(
+			$('link[rel=stylesheet]').map((_, el) => {
+				return fixture.readFile(el.attribs.href.replace(/\/blog/, ''));
+			})
+		);
+		const css = stylesheets.join('');
 
 		expect(css).to.match(/yellowgreen/, 'Svelte styles are added');
 		expect(css).to.match(/Courier New/, 'Global styles are added');

packages/astro/test/css-order-import.test.js

@@ -106,6 +106,25 @@ describe('CSS ordering - import order', () => {
 			expect(idx1).to.be.greaterThan(idx2);
 			expect(idx2).to.be.greaterThan(idx3);
 		});
+
+		it('correctly chunks css import from framework components', async () => {
+			let html = await fixture.readFile('/index.html');
+
+			const content = await Promise.all(getLinks(html).map((href) => getLinkContent(href)));
+			const [, { css }] = content;
+			expect(css).to.not.include(
+				'.client-1{background:red!important}',
+				'CSS from Client2.jsx leaked into index.astro when chunking'
+			);
+		});
+
+		it('dedupe css between astro and framework components', async () => {
+			let html = await fixture.readFile('/dedupe/index.html');
+
+			const content = await Promise.all(getLinks(html).map((href) => getLinkContent(href)));
+			const css = content.map((c) => c.css).join('');
+			expect(css.match(/\.astro-jsx/)?.length).to.eq(1, '.astro-jsx class is duplicated');
+		});
 	});
 
 	describe('Dynamic import', () => {

packages/astro/test/fixtures/css-order-import/astro.config.mjs

@@ -0,0 +1,7 @@
+import { defineConfig } from 'astro/config';
+import react from '@astrojs/react'
+
+// https://astro.build/config
+export default defineConfig({
+  integrations: [react()]
+});

packages/astro/test/fixtures/css-order-import/package.json

@@ -1,6 +1,9 @@
 {
   "name": "@test/css-order-import",
   "dependencies": {
-    "astro": "workspace:*"
+    "@astrojs/react": "workspace:*",
+    "astro": "workspace:*",
+    "react": "^18.1.0",
+    "react-dom": "^18.1.0"
   }
 }

packages/astro/test/fixtures/css-order-import/src/components/Client1.jsx

@@ -0,0 +1,5 @@
+import "../styles/Client1.css"
+
+export default function Client() {
+  return <div className="client-1">Client 1</div>;
+}

packages/astro/test/fixtures/css-order-import/src/components/Client2.jsx

@@ -0,0 +1,5 @@
+import "../styles/Client2.css"
+
+export default function Client() {
+  return <div className="client-2">Client 2</div>;
+}

packages/astro/test/fixtures/css-order-import/src/components/Dedupe.jsx

@@ -0,0 +1,5 @@
+import '../styles/AstroJsx.css';
+
+export default function Dedupe() {
+  return <div className="astro-jsx">Dedupe Astro JSX</div>;
+}

packages/astro/test/fixtures/css-order-import/src/pages/component.astro

@@ -1,6 +1,7 @@
 ---
 import One from '../components/One.astro';
 import Two from '../components/Two.astro';
+import Client2 from '../components/Client2.jsx';
 ---
 <html>
 <head>
@@ -15,5 +16,6 @@ import Two from '../components/Two.astro';
 </head>
 <body>
 	<h1>Test</h1>
+	<Client2 client:only="react" />
 </body>
 </html>

packages/astro/test/fixtures/css-order-import/src/pages/dedupe.astro

@@ -0,0 +1,13 @@
+---
+import '../styles/AstroJsx.css';
+import Dedupe from '../components/Dedupe.jsx';
+---
+<html>
+<head>
+	<title>Test</title>
+</head>
+<body>
+	<h1>Test</h1>
+	<Dedupe client:only="react" />
+</body>
+</html>

packages/astro/test/fixtures/css-order-import/src/pages/index.astro

@@ -1,5 +1,6 @@
 ---
 import '../styles/base.css';
+import Client1 from '../components/Client1.jsx';
 ---
 <html>
 <head>
@@ -12,5 +13,6 @@ import '../styles/base.css';
 </head>
 <body>
 	<h1>Test</h1>
+	<Client1 client:only="react" />
 </body>
 </html>

packages/astro/test/fixtures/css-order-import/src/styles/AstroJsx.css

@@ -0,0 +1,4 @@
+/* this css is imported in both .astro and .jsx component, make sure CSS is deduped */
+.astro-jsx {
+  color: red;
+}

packages/astro/test/fixtures/css-order-import/src/styles/Client1.css

@@ -0,0 +1,3 @@
+.client-1 {
+	background: green;
+}

packages/astro/test/fixtures/css-order-import/src/styles/Client2.css

@@ -0,0 +1,10 @@
+/*
+	This cheeky css tries to affect index.astro, the good thing is that the Client2.jsx component is
+	only loaded in component.astro. So index.astro's Client1.jsx component should not be affected by this.
+*/
+.client-1 {
+	background: red !important;
+}
+.client-2 {
+	background: green;
+}