Hoist Astro.globbed hoisted scripts in dev (#3930)

* Hoist Astro.globbed hoisted scripts in dev

* Adds a changeset

* Increase the timeout for the HMR test

* Fix e2e tests

* Refactor test

.changeset/quiet-toys-type.md

@@ -0,0 +1,5 @@
+---
+'astro': patch
+---
+
+Include hoisted scripts inside Astro.glob in dev

packages/astro/e2e/shared-component-tests.js

@@ -117,7 +117,7 @@ export function prepareTestFactory(opts) {
 				original.replace('id="client-idle" {...someProps}', 'id="client-idle" count={5}')
 			);
 
-			await expect(count, 'count prop updated').toHaveText('5');
+			await expect(count, 'count prop updated').toHaveText('5', { timeout: 10000 });
 			await expect(counter, 'component styles persisted').toHaveCSS('display', 'grid');
 
 			// Edit the client:only component's slot text

packages/astro/src/core/render/dev/css.ts

@@ -2,14 +2,9 @@ import type * as vite from 'vite';
 
 import path from 'path';
 import { RuntimeMode } from '../../../@types/astro.js';
-import { unwrapId, viteID } from '../../util.js';
+import { viteID } from '../../util.js';
 import { STYLE_EXTENSIONS } from '../util.js';
-
-/**
- * List of file extensions signalling we can (and should) SSR ahead-of-time
- * See usage below
- */
-const fileExtensionsToSSR = new Set(['.md']);
+import { crawlGraph } from './vite.js';
 
 /** Given a filePath URL, crawl Vite’s module graph to find all style imports. */
 export async function getStylesForURL(
@@ -20,69 +15,21 @@ export async function getStylesForURL(
 	const importedCssUrls = new Set<string>();
 	const importedStylesMap = new Map<string, string>();
 
-	/** recursively crawl the module graph to get all style files imported by parent id */
-	async function crawlCSS(_id: string, isFile: boolean, scanned = new Set<string>()) {
-		const id = unwrapId(_id);
-		const importedModules = new Set<vite.ModuleNode>();
-		const moduleEntriesForId = isFile
-			? // If isFile = true, then you are at the root of your module import tree.
-			  // The `id` arg is a filepath, so use `getModulesByFile()` to collect all
-			  // nodes for that file. This is needed for advanced imports like Tailwind.
-			  viteServer.moduleGraph.getModulesByFile(id) ?? new Set()
-			: // Otherwise, you are following an import in the module import tree.
-			  // You are safe to use getModuleById() here because Vite has already
-			  // resolved the correct `id` for you, by creating the import you followed here.
-			  new Set([viteServer.moduleGraph.getModuleById(id)]);
-
-		// Collect all imported modules for the module(s).
-		for (const entry of moduleEntriesForId) {
-			// Handle this in case an module entries weren't found for ID
-			// This seems possible with some virtual IDs (ex: `astro:markdown/*.md`)
-			if (!entry) {
-				continue;
+	for await(const importedModule of crawlGraph(viteServer, viteID(filePath), true)) {
+		const ext = path.extname(importedModule.url).toLowerCase();
+		if (STYLE_EXTENSIONS.has(ext)) {
+			if (
+				mode === 'development' && // only inline in development
+				typeof importedModule.ssrModule?.default === 'string' // ignore JS module styles
+			) {
+				importedStylesMap.set(importedModule.url, importedModule.ssrModule.default);
+			} else {
+				// NOTE: We use the `url` property here. `id` would break Windows.
+				importedCssUrls.add(importedModule.url);
 			}
-			if (id === entry.id) {
-				scanned.add(id);
-				for (const importedModule of entry.importedModules) {
-					// some dynamically imported modules are *not* server rendered in time
-					// to only SSR modules that we can safely transform, we check against
-					// a list of file extensions based on our built-in vite plugins
-					if (importedModule.id) {
-						// use URL to strip special query params like "?content"
-						const { pathname } = new URL(`file://${importedModule.id}`);
-						if (fileExtensionsToSSR.has(path.extname(pathname))) {
-							await viteServer.ssrLoadModule(importedModule.id);
-						}
-					}
-					importedModules.add(importedModule);
-				}
-			}
-		}
-
-		// scan imported modules for CSS imports & add them to our collection.
-		// Then, crawl that file to follow and scan all deep imports as well.
-		for (const importedModule of importedModules) {
-			if (!importedModule.id || scanned.has(importedModule.id)) {
-				continue;
-			}
-			const ext = path.extname(importedModule.url).toLowerCase();
-			if (STYLE_EXTENSIONS.has(ext)) {
-				if (
-					mode === 'development' && // only inline in development
-					typeof importedModule.ssrModule?.default === 'string' // ignore JS module styles
-				) {
-					importedStylesMap.set(importedModule.url, importedModule.ssrModule.default);
-				} else {
-					// NOTE: We use the `url` property here. `id` would break Windows.
-					importedCssUrls.add(importedModule.url);
-				}
-			}
-			await crawlCSS(importedModule.id, false, scanned);
 		}
 	}
 
-	// Crawl your import graph for CSS files, populating `importedCssUrls` as a result.
-	await crawlCSS(viteID(filePath), true);
 	return {
 		urls: importedCssUrls,
 		stylesMap: importedStylesMap,

packages/astro/src/core/render/dev/index.ts

@@ -14,9 +14,9 @@ import { LogOptions } from '../../logger/core.js';
 import { isBuildingToSSR, isPage } from '../../util.js';
 import { render as coreRender } from '../core.js';
 import { RouteCache } from '../route-cache.js';
-import { createModuleScriptElementWithSrcSet } from '../ssr-element.js';
 import { collectMdMetadata } from '../util.js';
 import { getStylesForURL } from './css.js';
+import { getScriptsForURL } from './scripts.js';
 import { resolveClientDevPath } from './resolve.js';
 
 export interface SSROptions {
@@ -108,9 +108,7 @@ export async function render(
 		viteServer,
 	} = ssrOpts;
 	// Add hoisted script tags
-	const scripts = createModuleScriptElementWithSrcSet(
-		mod.hasOwnProperty('$$metadata') ? Array.from(mod.$$metadata.hoistedScriptPaths()) : []
-	);
+	const scripts = await getScriptsForURL(filePath, astroConfig, viteServer);
 
 	// Inject HMR scripts
 	if (isPage(filePath, astroConfig) && mode === 'development') {

packages/astro/src/core/render/dev/scripts.ts

@@ -0,0 +1,44 @@
+import type { AstroConfig, SSRElement } from '../../../@types/astro';
+import type { ModuleInfo } from 'rollup';
+import type { PluginMetadata as AstroPluginMetadata } from '../../../vite-plugin-astro/types';
+import vite from 'vite';
+import slash from 'slash';
+import { viteID } from '../../util.js';
+import { createModuleScriptElementWithSrc } from '../ssr-element.js';
+import { crawlGraph } from './vite.js';
+import { fileURLToPath } from 'url';
+
+export async function getScriptsForURL(
+	filePath: URL,
+	astroConfig: AstroConfig,
+	viteServer: vite.ViteDevServer,
+): Promise<Set<SSRElement>> {
+	const elements = new Set<SSRElement>();
+	const rootID = viteID(filePath);
+	let rootProjectFolder = slash(fileURLToPath(astroConfig.root));
+	const modInfo = viteServer.pluginContainer.getModuleInfo(rootID);
+	addHoistedScripts(elements, modInfo, rootProjectFolder);
+	for await(const moduleNode of crawlGraph(viteServer, rootID, true)) {
+		const id = moduleNode.id;
+		if(id) {
+			const info = viteServer.pluginContainer.getModuleInfo(id);
+			addHoistedScripts(elements, info, rootProjectFolder);
+		}
+	}
+
+	return elements;
+}
+
+function addHoistedScripts(set: Set<SSRElement>, info: ModuleInfo | null, rootProjectFolder: string) {
+	if(!info?.meta?.astro) {
+		return;
+	}
+
+	let id = info.id;
+	const astro = info?.meta?.astro as AstroPluginMetadata['astro'];
+	for(let i = 0; i < astro.scripts.length; i++) {
+		const scriptId = `${id}?astro&type=script&index=${i}&lang.ts`;
+		const element = createModuleScriptElementWithSrc(scriptId);
+		set.add(element);
+	}
+}

packages/astro/src/core/render/dev/vite.ts

@@ -0,0 +1,68 @@
+import vite from 'vite';
+import npath from 'path';
+import { unwrapId } from '../../util.js';
+
+/**
+ * List of file extensions signalling we can (and should) SSR ahead-of-time
+ * See usage below
+ */
+ const fileExtensionsToSSR = new Set(['.astro', '.md']);
+
+/** recursively crawl the module graph to get all style files imported by parent id */
+export async function * crawlGraph(
+	viteServer: vite.ViteDevServer,
+	_id: string,
+	isFile: boolean,
+	scanned = new Set<string>()
+) : AsyncGenerator<vite.ModuleNode, void, unknown> {
+	const id = unwrapId(_id);
+	const importedModules = new Set<vite.ModuleNode>();
+	const moduleEntriesForId = isFile
+		? // If isFile = true, then you are at the root of your module import tree.
+			// The `id` arg is a filepath, so use `getModulesByFile()` to collect all
+			// nodes for that file. This is needed for advanced imports like Tailwind.
+			viteServer.moduleGraph.getModulesByFile(id) ?? new Set()
+		: // Otherwise, you are following an import in the module import tree.
+			// You are safe to use getModuleById() here because Vite has already
+			// resolved the correct `id` for you, by creating the import you followed here.
+			new Set([viteServer.moduleGraph.getModuleById(id)]);
+
+	// Collect all imported modules for the module(s).
+	for (const entry of moduleEntriesForId) {
+		// Handle this in case an module entries weren't found for ID
+		// This seems possible with some virtual IDs (ex: `astro:markdown/*.md`)
+		if (!entry) {
+			continue;
+		}
+		if (id === entry.id) {
+			scanned.add(id);
+			for (const importedModule of entry.importedModules) {
+				// some dynamically imported modules are *not* server rendered in time
+				// to only SSR modules that we can safely transform, we check against
+				// a list of file extensions based on our built-in vite plugins
+				if (importedModule.id) {
+					// use URL to strip special query params like "?content"
+					const { pathname } = new URL(`file://${importedModule.id}`);
+					if (fileExtensionsToSSR.has(npath.extname(pathname))) {
+						const mod = viteServer.moduleGraph.getModuleById(importedModule.id);
+						if(!mod?.ssrModule) {
+							await viteServer.ssrLoadModule(importedModule.id);
+						}
+					}
+				}
+				importedModules.add(importedModule);
+			}
+		}
+	}
+
+	// scan imported modules for CSS imports & add them to our collection.
+	// Then, crawl that file to follow and scan all deep imports as well.
+	for (const importedModule of importedModules) {
+		if (!importedModule.id || scanned.has(importedModule.id)) {
+			continue;
+		}
+
+		yield importedModule;
+		yield * crawlGraph(viteServer, importedModule.id, false, scanned);
+	}
+}

packages/astro/src/runtime/server/index.ts

@@ -825,7 +825,7 @@ export function renderHead(result: SSRResult): Promise<string> {
 	const scripts = Array.from(result.scripts)
 		.filter(uniqueElements)
 		.map((script, i) => {
-			return renderElement('script', script);
+			return renderElement('script', script, false);
 		});
 	const links = Array.from(result.links)
 		.filter(uniqueElements)

packages/astro/src/runtime/server/metadata.ts

@@ -58,39 +58,6 @@ export class Metadata {
 		return metadata?.componentExport || null;
 	}
 
-	*hoistedScriptPaths() {
-		for (const metadata of this.deepMetadata()) {
-			let i = 0,
-				pathname = metadata.mockURL.pathname;
-
-			while (i < metadata.hoisted.length) {
-				// Strip off the leading "/@fs" added during compilation.
-				yield `${pathname.replace('/@fs', '')}?astro&type=script&index=${i}&lang.ts`;
-				i++;
-			}
-		}
-	}
-
-	private *deepMetadata(): Generator<Metadata, void, unknown> {
-		// Yield self
-		yield this;
-		// Keep a Set of metadata objects so we only yield them out once.
-		const seen = new Set<Metadata>();
-		for (const { module: mod } of this.modules) {
-			if (typeof mod.$$metadata !== 'undefined') {
-				const md = mod.$$metadata as Metadata;
-				// Call children deepMetadata() which will yield the child metadata
-				// and any of its children metadatas
-				for (const childMetdata of md.deepMetadata()) {
-					if (!seen.has(childMetdata)) {
-						seen.add(childMetdata);
-						yield childMetdata;
-					}
-				}
-			}
-		}
-	}
-
 	private getComponentMetadata(Component: any): ComponentMetadata | null {
 		if (this.metadataCache.has(Component)) {
 			return this.metadataCache.get(Component)!;

packages/astro/test/astro-scripts.test.js

@@ -4,7 +4,6 @@ import { loadFixture } from './test-utils.js';
 
 describe('Scripts (hoisted and not)', () => {
 	let fixture;
-
 	before(async () => {
 		fixture = await loadFixture({
 			root: './fixtures/astro-scripts/',
@@ -14,94 +13,133 @@ describe('Scripts (hoisted and not)', () => {
 				},
 			},
 		});
-		await fixture.build();
 	});
 
-	it('Moves external scripts up', async () => {
-		const html = await fixture.readFile('/external/index.html');
-		const $ = cheerio.load(html);
-
-		expect($('head script[type="module"]:not([src="/regular_script.js"])')).to.have.lengthOf(1);
-		expect($('body script')).to.have.lengthOf(0);
+	describe('Build', () => {
+		before(async () => {
+			await fixture.build();
+		});
+	
+		it('Moves external scripts up', async () => {
+			const html = await fixture.readFile('/external/index.html');
+			const $ = cheerio.load(html);
+	
+			expect($('head script[type="module"]:not([src="/regular_script.js"])')).to.have.lengthOf(1);
+			expect($('body script')).to.have.lengthOf(0);
+		});
+	
+		it('Moves inline scripts up', async () => {
+			const html = await fixture.readFile('/inline/index.html');
+			const $ = cheerio.load(html);
+	
+			expect($('head script[type="module"]')).to.have.lengthOf(1);
+			expect($('body script')).to.have.lengthOf(0);
+		});
+	
+		it('Inline page builds the scripts to a single bundle', async () => {
+			// Inline page
+			let inline = await fixture.readFile('/inline/index.html');
+			let $ = cheerio.load(inline);
+			let $el = $('script');
+	
+			// test 1: Just one entry module
+			expect($el).to.have.lengthOf(1);
+	
+			// test 2: attr removed
+			expect($el.attr('data-astro')).to.equal(undefined);
+	
+			expect($el.attr('src')).to.equal(undefined);
+			const inlineEntryJS = $el.text();
+	
+			// test 3: the JS exists
+			expect(inlineEntryJS).to.be.ok;
+	
+			// test 4: Inline imported JS is included
+			expect(inlineEntryJS).to.contain(
+				'I AM IMPORTED INLINE',
+				'The inline imported JS is included in the bundle'
+			);
+		});
+	
+		it("Inline scripts that are shared by multiple pages create chunks, and aren't inlined into the HTML", async () => {
+			let html = await fixture.readFile('/inline-shared-one/index.html');
+			let $ = cheerio.load(html);
+	
+			expect($('script')).to.have.lengthOf(1);
+			expect($('script').attr('src')).to.not.equal(undefined);
+		});
+	
+		it('External page builds the hoisted scripts to a single bundle', async () => {
+			let external = await fixture.readFile('/external/index.html');
+			let $ = cheerio.load(external);
+	
+			// test 1: there are two scripts
+			expect($('script')).to.have.lengthOf(2);
+	
+			let el = $('script').get(1);
+			expect($(el).attr('src')).to.equal(undefined, 'This should have been inlined');
+			let externalEntryJS = $(el).text();
+	
+			// test 2: the JS exists
+			expect(externalEntryJS).to.be.ok;
+		});
+	
+		it('External page using non-hoist scripts that are modules are built standalone', async () => {
+			let external = await fixture.readFile('/external-no-hoist/index.html');
+			let $ = cheerio.load(external);
+	
+			// test 1: there is 1 scripts
+			expect($('script')).to.have.lengthOf(1);
+	
+			// test 2: inside assets
+			let entryURL = $('script').attr('src');
+			expect(entryURL.includes('assets/')).to.equal(true);
+		});
+	
+		it('External page using non-hoist scripts that are not modules are built standalone', async () => {
+			let external = await fixture.readFile('/external-no-hoist-classic/index.html');
+			let $ = cheerio.load(external);
+	
+			// test 1: there is 1 scripts
+			expect($('script')).to.have.lengthOf(1);
+	
+			// test 2: inside assets
+			let entryURL = $('script').attr('src');
+			expect(entryURL.includes('assets/')).to.equal(true);
+		});
+	
+		it('Scripts added via Astro.glob are hoisted', async () => {
+			let glob = await fixture.readFile('/glob/index.html');
+			let $ = cheerio.load(glob);
+	
+			expect($('script[type="module"]').length).to.be.greaterThan(0);
+		});
 	});
 
-	it('Moves inline scripts up', async () => {
-		const html = await fixture.readFile('/inline/index.html');
-		const $ = cheerio.load(html);
+	describe('Dev', () => {
+		/** @type {import('./test-utils').DevServer} */
+		let devServer;
+		before(async () => {
+			devServer = await fixture.startDevServer();
+		});
 
-		expect($('head script[type="module"]')).to.have.lengthOf(1);
-		expect($('body script')).to.have.lengthOf(0);
-	});
+		after(async () => {
+			await devServer.stop();
+		});
 
-	it('Inline page builds the scripts to a single bundle', async () => {
-		// Inline page
-		let inline = await fixture.readFile('/inline/index.html');
-		let $ = cheerio.load(inline);
-		let $el = $('script');
+		it('Scripts added via Astro.glob are hoisted', async () => {
+			let res = await fixture.fetch('/glob');
+			let html = await res.text();
+			let $ = cheerio.load(html);
 
-		// test 1: Just one entry module
-		expect($el).to.have.lengthOf(1);
-
-		// test 2: attr removed
-		expect($el.attr('data-astro')).to.equal(undefined);
-
-		expect($el.attr('src')).to.equal(undefined);
-		const inlineEntryJS = $el.text();
-
-		// test 3: the JS exists
-		expect(inlineEntryJS).to.be.ok;
-
-		// test 4: Inline imported JS is included
-		expect(inlineEntryJS).to.contain(
-			'I AM IMPORTED INLINE',
-			'The inline imported JS is included in the bundle'
-		);
-	});
-
-	it("Inline scripts that are shared by multiple pages create chunks, and aren't inlined into the HTML", async () => {
-		let html = await fixture.readFile('/inline-shared-one/index.html');
-		let $ = cheerio.load(html);
-
-		expect($('script')).to.have.lengthOf(1);
-		expect($('script').attr('src')).to.not.equal(undefined);
-	});
-
-	it('External page builds the hoisted scripts to a single bundle', async () => {
-		let external = await fixture.readFile('/external/index.html');
-		let $ = cheerio.load(external);
-
-		// test 1: there are two scripts
-		expect($('script')).to.have.lengthOf(2);
-
-		let el = $('script').get(1);
-		expect($(el).attr('src')).to.equal(undefined, 'This should have been inlined');
-		let externalEntryJS = $(el).text();
-
-		// test 2: the JS exists
-		expect(externalEntryJS).to.be.ok;
-	});
-
-	it('External page using non-hoist scripts that are modules are built standalone', async () => {
-		let external = await fixture.readFile('/external-no-hoist/index.html');
-		let $ = cheerio.load(external);
-
-		// test 1: there is 1 scripts
-		expect($('script')).to.have.lengthOf(1);
-
-		// test 2: inside assets
-		let entryURL = $('script').attr('src');
-		expect(entryURL.includes('assets/')).to.equal(true);
-	});
-
-	it('External page using non-hoist scripts that are not modules are built standalone', async () => {
-		let external = await fixture.readFile('/external-no-hoist-classic/index.html');
-		let $ = cheerio.load(external);
-
-		// test 1: there is 1 scripts
-		expect($('script')).to.have.lengthOf(1);
-
-		// test 2: inside assets
-		let entryURL = $('script').attr('src');
-		expect(entryURL.includes('assets/')).to.equal(true);
+			let found = 0;
+			let moduleScripts = $('[type=module]');
+			moduleScripts.each((i, el) => {
+				if($(el).attr('src').includes('Glob/GlobComponent.astro?astro&type=script&index=0&lang.ts')) {
+					found++;
+				}
+			})
+			expect(found).to.equal(1);
+		});
 	});
 });

packages/astro/test/fixtures/astro-scripts/src/components/Glob/GlobComponent.astro

@@ -0,0 +1,4 @@
+<script>
+  console.log(`Boom! ${new Date()}`);
+</script>
+<h2>My Component</h2>

packages/astro/test/fixtures/astro-scripts/src/pages/glob.astro

@@ -0,0 +1,16 @@
+---
+const components = await Astro.glob('/src/components/Glob/*');
+const MyComponent = components[0].default;
+---
+
+<html lang="en">
+	<head>
+		<meta charset="utf-8" />
+		<meta name="viewport" content="width=device-width" />
+		<title>Astro</title>
+	</head>
+	<body>
+		<p>My Component is below...</p>
+		<MyComponent />
+	</body>
+</html>