Fix CSS chunking between multiple framework components (#6582)

* Fix CSS chunking between multiple framework components

* Better CSS dedupe handling

* Add more tests

* Update docs

16 files changed, 115 insertions, 16 deletions

diff --git a/.changeset/violet-islands-buy.md b/.changeset/violet-islands-buy.md
new file mode 100644
index 000000000..8c88d23be
--- /dev/null
+++ b/.changeset/violet-islands-buy.md
@@ -0,0 +1,5 @@
+---
+'astro': patch
+---
+
+Fix CSS chunking and deduping between multiple Astro files and framework components
diff --git a/packages/astro/src/core/build/plugins/plugin-css.ts b/packages/astro/src/core/build/plugins/plugin-css.ts
index 7a5900460..2df111487 100644
--- a/packages/astro/src/core/build/plugins/plugin-css.ts
+++ b/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
diff --git a/packages/astro/test/astro-client-only.test.js b/packages/astro/test/astro-client-only.test.js
index f3329c683..c003e0a28 100644
--- a/packages/astro/test/astro-client-only.test.js
+++ b/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');
diff --git a/packages/astro/test/css-order-import.test.js b/packages/astro/test/css-order-import.test.js
index 01b6f5497..745187c53 100644
--- a/packages/astro/test/css-order-import.test.js
+++ b/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', () => {
diff --git a/packages/astro/test/fixtures/css-order-import/astro.config.mjs b/packages/astro/test/fixtures/css-order-import/astro.config.mjs
new file mode 100644
index 000000000..1c9fe511f
--- /dev/null
+++ b/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()]
+});
diff --git a/packages/astro/test/fixtures/css-order-import/package.json b/packages/astro/test/fixtures/css-order-import/package.json
index 2901a838f..caf2346ea 100644
--- a/packages/astro/test/fixtures/css-order-import/package.json
+++ b/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"
   }
 }
diff --git a/packages/astro/test/fixtures/css-order-import/src/components/Client1.jsx b/packages/astro/test/fixtures/css-order-import/src/components/Client1.jsx
new file mode 100644
index 000000000..2cec90a9a
--- /dev/null
+++ b/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>;
+}
diff --git a/packages/astro/test/fixtures/css-order-import/src/components/Client2.jsx b/packages/astro/test/fixtures/css-order-import/src/components/Client2.jsx
new file mode 100644
index 000000000..226f54679
--- /dev/null
+++ b/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>;
+}
diff --git a/packages/astro/test/fixtures/css-order-import/src/components/Dedupe.jsx b/packages/astro/test/fixtures/css-order-import/src/components/Dedupe.jsx
new file mode 100644
index 000000000..8b8889251
--- /dev/null
+++ b/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>;
+}
diff --git a/packages/astro/test/fixtures/css-order-import/src/pages/component.astro b/packages/astro/test/fixtures/css-order-import/src/pages/component.astro
index 018ab1866..1b779d18d 100644
--- a/packages/astro/test/fixtures/css-order-import/src/pages/component.astro
+++ b/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>
diff --git a/packages/astro/test/fixtures/css-order-import/src/pages/dedupe.astro b/packages/astro/test/fixtures/css-order-import/src/pages/dedupe.astro
new file mode 100644
index 000000000..ea3a01670
--- /dev/null
+++ b/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>
diff --git a/packages/astro/test/fixtures/css-order-import/src/pages/index.astro b/packages/astro/test/fixtures/css-order-import/src/pages/index.astro
index 0843250c0..0a6baab59 100644
--- a/packages/astro/test/fixtures/css-order-import/src/pages/index.astro
+++ b/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>
diff --git a/packages/astro/test/fixtures/css-order-import/src/styles/AstroJsx.css b/packages/astro/test/fixtures/css-order-import/src/styles/AstroJsx.css
new file mode 100644
index 000000000..c69af5608
--- /dev/null
+++ b/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;
+}
diff --git a/packages/astro/test/fixtures/css-order-import/src/styles/Client1.css b/packages/astro/test/fixtures/css-order-import/src/styles/Client1.css
new file mode 100644
index 000000000..86eb7de5d
--- /dev/null
+++ b/packages/astro/test/fixtures/css-order-import/src/styles/Client1.css
@@ -0,0 +1,3 @@
+.client-1 {
+	background: green;
+}
diff --git a/packages/astro/test/fixtures/css-order-import/src/styles/Client2.css b/packages/astro/test/fixtures/css-order-import/src/styles/Client2.css
new file mode 100644
index 000000000..10acac0f3
--- /dev/null
+++ b/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;
+}
diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml
index 2d961d786..d623f0514 100644
--- a/pnpm-lock.yaml
+++ b/pnpm-lock.yaml
@@ -1949,9 +1949,15 @@ importers:
 
   packages/astro/test/fixtures/css-order-import:
     specifiers:
+      '@astrojs/react': workspace:*
       astro: workspace:*
+      react: ^18.1.0
+      react-dom: ^18.1.0
     dependencies:
+      '@astrojs/react': link:../../../../integrations/react
       astro: link:../../..
+      react: 18.2.0
+      react-dom: 18.2.0_react@18.2.0
 
   packages/astro/test/fixtures/css-order-layout:
     specifiers: