Resolve components by module ID during compilation (#3300)

* WIP: adding test coverage

* test fixes

* moving the shared lib up a directory to reproduce the bug

* fix: transform with the module ID instead of parsing the filepath

* adding the shared lib to the workspaces list

* fix: client-only assets now get the full URL from vite

* why is this needed for windows?

* WIP: using /@fs to handle windows filepaths

* fix: remove /@fs from hoisted script imports

* nit: removing unused imports

* fix: strip off the path root when mapping client:only styles

* had to reverse the `/@fs` handling to work on windows and unix

* chore: adding comments to explain the fix

* chore: adding changeset

.changeset/proud-cups-brush.md

@@ -0,0 +1,5 @@
+---
+'astro': patch
+---
+
+Resolve .astro components by module ID to support the use of Astro + framework components in an NPM package

package.json

@@ -33,8 +33,7 @@
     "examples/component/packages/*",
     "scripts",
     "smoke/*",
-    "packages/astro/test/fixtures/builtins/packages/*",
+    "packages/astro/test/fixtures/component-library-shared",
-    "packages/astro/test/fixtures/builtins-polyfillnode",
     "packages/astro/test/fixtures/custom-elements/my-component-lib",
     "packages/astro/test/fixtures/static build/pkg"
   ],

packages/astro/src/core/build/internal.ts

@@ -1,8 +1,7 @@
-import type { AstroConfig, RouteData } from '../../@types/astro';
 import type { RenderedChunk } from 'rollup';
 import type { PageBuildData, ViteID } from './types';
 
-import { fileURLToPath } from 'url';
+import { prependForwardSlash } from '../path.js';
 import { viteID } from '../util.js';
 
 export interface BuildInternals {
@@ -80,17 +79,16 @@ export function trackPageData(
 export function trackClientOnlyPageDatas(
 	internals: BuildInternals,
 	pageData: PageBuildData,
-	clientOnlys: string[],
+	clientOnlys: string[]
-	astroConfig: AstroConfig
 ) {
 	for (const clientOnlyComponent of clientOnlys) {
-		const coPath = viteID(new URL('.' + clientOnlyComponent, astroConfig.root));
 		let pageDataSet: Set<PageBuildData>;
-		if (internals.pagesByClientOnly.has(coPath)) {
+		// clientOnlyComponent will be similar to `/@fs{moduleID}`
-			pageDataSet = internals.pagesByClientOnly.get(coPath)!;
+		if (internals.pagesByClientOnly.has(clientOnlyComponent)) {
+			pageDataSet = internals.pagesByClientOnly.get(clientOnlyComponent)!;
 		} else {
 			pageDataSet = new Set<PageBuildData>();
-			internals.pagesByClientOnly.set(coPath, pageDataSet);
+			internals.pagesByClientOnly.set(clientOnlyComponent, pageDataSet);
 		}
 		pageDataSet.add(pageData);
 	}
@@ -115,8 +113,10 @@ export function* getPageDatasByClientOnlyChunk(
 	const pagesByClientOnly = internals.pagesByClientOnly;
 	if (pagesByClientOnly.size) {
 		for (const [modulePath] of Object.entries(chunk.modules)) {
-			if (pagesByClientOnly.has(modulePath)) {
+			// prepend with `/@fs` to match the path used in the compiler's transform() call
-				for (const pageData of pagesByClientOnly.get(modulePath)!) {
+			const pathname = `/@fs${prependForwardSlash(modulePath)}`;
+			if (pagesByClientOnly.has(pathname)) {
+				for (const pageData of pagesByClientOnly.get(pathname)!) {
 					yield pageData;
 				}
 			}

packages/astro/src/core/build/static-build.ts

@@ -56,7 +56,7 @@ export async function staticBuild(opts: StaticBuildOptions) {
 
 			// Track client:only usage so we can map their CSS back to the Page they are used in.
 			const clientOnlys = Array.from(metadata.clientOnlyComponentPaths());
-			trackClientOnlyPageDatas(internals, pageData, clientOnlys, astroConfig);
+			trackClientOnlyPageDatas(internals, pageData, clientOnlys);
 
 			const topLevelImports = new Set([
 				// Any component that gets hydrated

packages/astro/src/core/build/vite-plugin-hoisted-scripts.ts

@@ -25,7 +25,12 @@ export function vitePluginHoistedScripts(
 			if (virtualHoistedEntry(id)) {
 				let code = '';
 				for (let path of internals.hoistedScriptIdToHoistedMap.get(id)!) {
-					code += `import "${path}";`;
+					let importPath = path;
+					// `/@fs` is added during the compiler's transform() step
+					if (importPath.startsWith('/@fs')) {
+						importPath = importPath.slice('/@fs'.length);
+					}
+					code += `import "${importPath}";`;
 				}
 				return {
 					code,

packages/astro/src/vite-plugin-astro/compile.ts

@@ -34,16 +34,25 @@ function safelyReplaceImportPlaceholder(code: string) {
 
 const configCache = new WeakMap<AstroConfig, CompilationCache>();
 
-async function compile(
+interface CompileProps {
-	config: AstroConfig,
+	config: AstroConfig;
-	filename: string,
+	filename: string;
-	source: string,
+	moduleId: string;
-	viteTransform: TransformHook,
+	source: string;
-	opts: { ssr: boolean }
+	ssr: boolean;
-): Promise<CompileResult> {
+	viteTransform: TransformHook;
+}
+
+async function compile({
+	config,
+	filename,
+	moduleId,
+	source,
+	ssr,
+	viteTransform,
+}: CompileProps): Promise<CompileResult> {
 	const filenameURL = new URL(`file://${filename}`);
 	const normalizedID = fileURLToPath(filenameURL);
-	const pathname = filenameURL.pathname.slice(config.root.pathname.length - 1);
 
 	let rawCSSDeps = new Set<string>();
 	let cssTransformError: Error | undefined;
@@ -52,7 +61,8 @@ async function compile(
 	// use `sourcemap: "both"` so that sourcemap is included in the code
 	// result passed to esbuild, but also available in the catch handler.
 	const transformResult = await transform(source, {
-		pathname,
+		// For Windows compat, prepend the module ID with `/@fs`
+		pathname: `/@fs${prependForwardSlash(moduleId)}`,
 		projectRoot: config.root.toString(),
 		site: config.site ? new URL(config.base, config.site).toString() : undefined,
 		sourcefile: filename,
@@ -86,7 +96,7 @@ async function compile(
 					lang,
 					id: normalizedID,
 					transformHook: viteTransform,
-					ssr: opts.ssr,
+					ssr,
 				});
 
 				let map: SourceMapInput | undefined;
@@ -131,13 +141,8 @@ export function invalidateCompilation(config: AstroConfig, filename: string) {
 	}
 }
 
-export async function cachedCompilation(
+export async function cachedCompilation(props: CompileProps): Promise<CompileResult> {
-	config: AstroConfig,
+	const { config, filename } = props;
-	filename: string,
-	source: string,
-	viteTransform: TransformHook,
-	opts: { ssr: boolean }
-): Promise<CompileResult> {
 	let cache: CompilationCache;
 	if (!configCache.has(config)) {
 		cache = new Map();
@@ -148,7 +153,7 @@ export async function cachedCompilation(
 	if (cache.has(filename)) {
 		return cache.get(filename)!;
 	}
-	const compileResult = await compile(config, filename, source, viteTransform, opts);
+	const compileResult = await compile(props);
 	cache.set(filename, compileResult);
 	return compileResult;
 }

packages/astro/src/vite-plugin-astro/index.ts

@@ -99,15 +99,21 @@ export default function astro({ config, logging }: AstroPluginOptions): vite.Plu
 			if (isPage && config._ctx.scripts.some((s) => s.stage === 'page')) {
 				source += `\n<script src="${PAGE_SCRIPT_ID}" />`;
 			}
+			const compileProps = {
+				config,
+				filename,
+				moduleId: id,
+				source,
+				ssr: Boolean(opts?.ssr),
+				viteTransform,
+			};
 			if (query.astro) {
 				if (query.type === 'style') {
 					if (typeof query.index === 'undefined') {
 						throw new Error(`Requests for Astro CSS must include an index.`);
 					}
 
-					const transformResult = await cachedCompilation(config, filename, source, viteTransform, {
+					const transformResult = await cachedCompilation(compileProps);
-						ssr: Boolean(opts?.ssr),
-					});
 
 					// Track any CSS dependencies so that HMR is triggered when they change.
 					await trackCSSDependencies.call(this, {
@@ -133,9 +139,7 @@ export default function astro({ config, logging }: AstroPluginOptions): vite.Plu
 						};
 					}
 
-					const transformResult = await cachedCompilation(config, filename, source, viteTransform, {
+					const transformResult = await cachedCompilation(compileProps);
-						ssr: Boolean(opts?.ssr),
-					});
 					const scripts = transformResult.scripts;
 					const hoistedScript = scripts[query.index];
 
@@ -163,9 +167,7 @@ export default function astro({ config, logging }: AstroPluginOptions): vite.Plu
 			}
 
 			try {
-				const transformResult = await cachedCompilation(config, filename, source, viteTransform, {
+				const transformResult = await cachedCompilation(compileProps);
-					ssr: Boolean(opts?.ssr),
-				});
 
 				// Compile all TypeScript to JavaScript.
 				// Also, catches invalid JS/TS in the compiled output before returning.

packages/astro/test/component-library.test.js

@@ -0,0 +1,157 @@
+import { expect } from 'chai';
+import { load as cheerioLoad } from 'cheerio';
+import { loadFixture } from './test-utils.js';
+
+function addLeadingSlash(path) {
+	return path.startsWith('/') ? path : '/' + path;
+}
+
+describe('Component Libraries', () => {
+	/** @type {import('./test-utils').Fixture} */
+	let fixture;
+
+	before(async () => {
+		fixture = await loadFixture({
+			root: './fixtures/component-library/',
+		});
+	});
+
+	describe('build', async () => {
+		before(async () => {
+			await fixture.build();
+		});
+
+		function createFindEvidence(expected, prefix) {
+			return async function findEvidence(pathname) {
+				const html = await fixture.readFile(pathname);
+				const $ = cheerioLoad(html);
+				const links = $('link[rel=stylesheet]');
+				for (const link of links) {
+					const href = $(link).attr('href');
+
+					const data = await fixture.readFile(addLeadingSlash(href));
+					if (expected.test(data)) {
+						return true;
+					}
+				}
+
+				return false;
+			};
+		}
+
+		it('Built .astro pages', async () => {
+			let html = await fixture.readFile('/with-astro/index.html');
+			expect(html).to.be.a('string');
+
+			html = await fixture.readFile('/with-react/index.html');
+			expect(html).to.be.a('string');
+
+			html = await fixture.readFile('/internal-hydration/index.html');
+			expect(html).to.be.a('string');
+		});
+
+		it('Works with .astro components', async () => {
+			const html = await fixture.readFile('/with-astro/index.html');
+			const $ = cheerioLoad(html);
+
+			expect($('button').text()).to.equal('Click me', "Rendered the component's slot");
+
+			const findEvidence = createFindEvidence(/border-radius:( )*1rem/);
+			expect(await findEvidence('with-astro/index.html')).to.equal(
+				true,
+				"Included the .astro component's <style>"
+			);
+		});
+
+		it('Works with react components', async () => {
+			const html = await fixture.readFile('/with-react/index.html');
+			const $ = cheerioLoad(html);
+
+			expect($('#react-static').text()).to.equal('Hello static!', 'Rendered the static component');
+
+			expect($('#react-idle').text()).to.equal(
+				'Hello idle!',
+				'Rendered the client hydrated component'
+			);
+
+			expect($('astro-root[uid]')).to.have.lengthOf(1, 'Included one hydration island');
+		});
+
+		it('Works with components hydrated internally', async () => {
+			const html = await fixture.readFile('/internal-hydration/index.html');
+			const $ = cheerioLoad(html);
+
+			expect($('.counter').length).to.equal(1, 'Rendered the svelte counter');
+			expect($('.counter-message').text().trim()).to.equal('Hello, Svelte!', "rendered the counter's slot");
+
+			expect($('astro-root[uid]')).to.have.lengthOf(1, 'Included one hydration island');
+		});
+	});
+
+	describe('dev', async () => {
+		let devServer;
+
+		before(async () => {
+			devServer = await fixture.startDevServer();
+		});
+
+		after(async () => {
+			await devServer.stop();
+		});
+
+		function createFindEvidence(expected, prefix) {
+			return async function findEvidence(pathname) {
+				const html = await fixture.fetch(pathname).then((res) => res.text());
+				const $ = cheerioLoad(html);
+				const links = $('link[rel=stylesheet]');
+				for (const link of links) {
+					const href = $(link).attr('href');
+
+					const data = await fixture.fetch(addLeadingSlash(href)).then((res) => res.text());
+					if (expected.test(data)) {
+						return true;
+					}
+				}
+
+				return false;
+			};
+		}
+
+		it('Works with .astro components', async () => {
+			const html = await fixture.fetch('/with-astro/').then((res) => res.text());
+			const $ = cheerioLoad(html);
+
+			expect($('button').text()).to.equal('Click me', "Rendered the component's slot");
+
+			const findEvidence = createFindEvidence(/border-radius:( )*1rem/);
+			expect(await findEvidence('/with-astro/')).to.equal(
+				true,
+				"Included the .astro component's <style>"
+			);
+		});
+
+		it('Works with react components', async () => {
+			const html = await fixture.fetch('/with-react/').then((res) => res.text());
+			const $ = cheerioLoad(html);
+
+			expect($('#react-static').text()).to.equal('Hello static!', 'Rendered the static component');
+
+			expect($('#react-idle').text()).to.equal(
+				'Hello idle!',
+				'Rendered the client hydrated component'
+			);
+
+			expect($('astro-root[uid]')).to.have.lengthOf(1, 'Included one hydration island');
+		});
+
+		it('Works with components hydrated internally', async () => {
+			const html = await fixture.fetch('/internal-hydration/').then((res) => res.text());
+			const $ = cheerioLoad(html);
+
+			expect($('.counter').length).to.equal(1, 'Rendered the svelte counter');
+			expect($('.counter-message').text().trim()).to.equal('Hello, Svelte!', "rendered the counter's slot");
+
+			expect($('astro-root[uid]')).to.have.lengthOf(1, 'Included one hydration island');
+		});
+	});
+});

packages/astro/test/fixtures/component-library-shared/Button.astro

@@ -0,0 +1,7 @@
+<button><slot /></button>
+
+<style>
+	button {
+		border-radius: 1rem;
+	}
+</style>

packages/astro/test/fixtures/component-library-shared/Counter.css

@@ -0,0 +1,11 @@
+.counter {
+	display: grid;
+	font-size: 2em;
+	grid-template-columns: repeat(3, minmax(0, 1fr));
+	margin-top: 2em;
+	place-items: center;
+}
+
+.counter-message {
+	text-align: center;
+}

packages/astro/test/fixtures/component-library-shared/Counter.jsx

@@ -0,0 +1,20 @@
+import { h } from 'preact';
+import { useState } from 'preact/hooks';
+import './Counter.css';
+
+export default function Counter({ children }) {
+	const [count, setCount] = useState(0);
+	const add = () => setCount((i) => i + 1);
+	const subtract = () => setCount((i) => i - 1);
+
+	return (
+		<>
+			<div class="counter">
+				<button onClick={subtract}>-</button>
+				<pre>{count}</pre>
+				<button onClick={add}>+</button>
+			</div>
+			<div class="counter-message">{children}</div>
+		</>
+	);
+}

packages/astro/test/fixtures/component-library-shared/Counter.svelte

@@ -0,0 +1,33 @@
+<script>
+  let count = 0;
+
+  function add() {
+    count += 1;
+  }
+
+  function subtract() {
+    count -= 1;
+  }
+</script>
+
+<div class="counter">
+  <button on:click={subtract}>-</button>
+  <pre>{ count }</pre>
+  <button on:click={add}>+</button>
+</div>
+<div class="message">
+  <slot />
+</div>
+
+<style>
+  .counter{
+    display: grid;
+    font-size: 2em;
+    grid-template-columns: repeat(3, minmax(0, 1fr));
+    margin-top: 2em;
+    place-items: center;
+  }
+  .message {
+    text-align: center;
+  }
+</style>

packages/astro/test/fixtures/component-library-shared/CounterWrapper.astro

@@ -0,0 +1,7 @@
+---
+import Counter from './Counter.jsx'
+---
+
+<Counter client:visible>
+	<slot />
+</Counter>

packages/astro/test/fixtures/component-library-shared/HelloReact.jsx

@@ -0,0 +1,5 @@
+import React from 'react';
+
+export default function ({ name, unused }) {
+  return <h2 id={`react-${name}`}>Hello {name}!</h2>;
+}

packages/astro/test/fixtures/component-library-shared/README.md

@@ -0,0 +1,5 @@
+# Shared Component Library
+
+> This packages mimics a component library that would be published to NPM
+
+This used by the `component-library` test fixture to make sure that `.astro` component are resolved properly when they live outside the project's root directory.

packages/astro/test/fixtures/component-library-shared/package.json

@@ -0,0 +1,20 @@
+{
+  "name": "@test/component-library-shared",
+  "version": "0.1.0",
+  "private": true,
+  "type": "module",
+  "exports": {
+    "./Button.astro": "./Button.astro",
+    "./CounterWrapper": "./CounterWrapper.astro",
+    "./HelloReact": "./HelloReact.jsx"
+  },
+  "files": [
+    "Button.astro",
+    "CounterWrapper.astro",
+    "Counter.svelte",
+    "HelloReact.jsx"
+  ],
+  "devDependencies": {
+    "astro": "workspace:*"
+  }
+}

packages/astro/test/fixtures/component-library/astro.config.mjs

@@ -0,0 +1,8 @@
+import { defineConfig } from 'astro/config';
+import preact from '@astrojs/preact';
+import react from '@astrojs/react';
+import svelte from '@astrojs/svelte';
+
+export default defineConfig({
+	integrations: [preact(), react(), svelte()],
+})

packages/astro/test/fixtures/component-library/package.json

@@ -0,0 +1,14 @@
+{
+  "name": "@test/component-library",
+  "version": "0.0.0",
+  "private": true,
+  "dependencies": {
+    "astro": "workspace:*",
+    "@astrojs/preact": "workspace:*",
+    "@astrojs/react": "workspace:*",
+    "@astrojs/svelte": "workspace:*",
+    "@test/component-library-shared": "workspace:*",
+    "react": "^18.0.0",
+    "react-dom": "^18.0.0"
+  }
+}

packages/astro/test/fixtures/component-library/src/pages/internal-hydration.astro

@@ -0,0 +1,17 @@
+---
+import CounterWrapper from '@test/component-library-shared/CounterWrapper';
+const title = 'With Astro Component';
+---
+
+<html>
+<head>
+  <title>{title}</title>
+</head>
+<body>
+  <h1>{title}</h1>
+
+  <CounterWrapper>
+		<h1>Hello, Svelte!</h1>
+	</CounterWrapper>
+</body>
+</html>

packages/astro/test/fixtures/component-library/src/pages/with-astro.astro

@@ -0,0 +1,15 @@
+---
+import Button from '@test/component-library-shared/Button.astro';
+const title = 'With Astro Component';
+---
+
+<html>
+<head>
+  <title>{title}</title>
+</head>
+<body>
+  <h1>{title}</h1>
+
+  <Button>Click me</Button>
+</body>
+</html>

packages/astro/test/fixtures/component-library/src/pages/with-react.astro

@@ -0,0 +1,7 @@
+---
+import HelloReact from '@test/component-library-shared/HelloReact';
+---
+
+<HelloReact name="static" />
+
+<HelloReact name="idle" client:idle />

pnpm-lock.yaml

@@ -951,6 +951,30 @@ importers:
     dependencies:
       astro: link:../../..
 
+  packages/astro/test/fixtures/component-library:
+    specifiers:
+      '@astrojs/preact': workspace:*
+      '@astrojs/react': workspace:*
+      '@astrojs/svelte': workspace:*
+      '@test/component-library-shared': workspace:*
+      astro: workspace:*
+      react: ^18.0.0
+      react-dom: ^18.0.0
+    dependencies:
+      '@astrojs/preact': link:../../../../integrations/preact
+      '@astrojs/react': link:../../../../integrations/react
+      '@astrojs/svelte': link:../../../../integrations/svelte
+      '@test/component-library-shared': link:../component-library-shared
+      astro: link:../../..
+      react: 18.0.0
+      react-dom: 18.0.0_react@18.0.0
+
+  packages/astro/test/fixtures/component-library-shared:
+    specifiers:
+      astro: workspace:*
+    devDependencies:
+      astro: link:../../..
+
   packages/astro/test/fixtures/config-host:
     specifiers:
       astro: workspace:*