Use base rather than site to create subpath for links/scripts (#5371)

* Use base rather than site to create subpath for links/scripts

* Adding a changeset

* Warn when the site has a pathname but not using base

* fix asset test

* fix asset inlining behavior

.changeset/thirty-cheetahs-repeat.md

@@ -0,0 +1,5 @@
+---
+'astro': patch
+---
+
+Prefer the `base` config rather than site config for creating URLs for links and scripts.

packages/astro/src/core/app/index.ts

@@ -170,7 +170,7 @@ export class App {
 		const url = new URL(request.url);
 		const manifest = this.#manifest;
 		const info = this.#routeDataToRouteInfo.get(routeData!)!;
-		const links = createLinkStylesheetElementSet(info.links, manifest.site);
+		const links = createLinkStylesheetElementSet(info.links);
 
 		let scripts = new Set<SSRElement>();
 		for (const script of info.scripts) {
@@ -182,7 +182,7 @@ export class App {
 					});
 				}
 			} else {
-				scripts.add(createModuleScriptElement(script, manifest.site));
+				scripts.add(createModuleScriptElement(script));
 			}
 		}
 

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

@@ -291,14 +291,8 @@ async function generatePath(
 
 	debug('build', `Generating: ${pathname}`);
 
-	// If a base path was provided, append it to the site URL. This ensures that
+	const links = createLinkStylesheetElementSet(linkIds, settings.config.base);
-	// all injected scripts and links are referenced relative to the site and subpath.
+	const scripts = createModuleScriptsSet(hoistedScripts ? [hoistedScripts] : [], settings.config.base);
-	const site =
-		settings.config.base !== '/'
-			? joinPaths(settings.config.site?.toString() || 'http://localhost/', settings.config.base)
-			: settings.config.site;
-	const links = createLinkStylesheetElementSet(linkIds, site);
-	const scripts = createModuleScriptsSet(hoistedScripts ? [hoistedScripts] : [], site);
 
 	if (settings.scripts.some((script) => script.stage === 'page')) {
 		const hashedFilePath = internals.entrySpecifierToBundleMap.get(PAGE_SCRIPT_ID);

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

@@ -40,7 +40,10 @@ export function vitePluginHoistedScripts(
 		},
 
 		async generateBundle(_options, bundle) {
-			let assetInlineLimit = settings.config.vite?.build?.assetsInlineLimit || 4096;
+			let assetInlineLimit = 4096;
+			if(settings.config.vite?.build && settings.config.vite.build.assetsInlineLimit !== undefined) {
+				assetInlineLimit = settings.config.vite?.build.assetsInlineLimit;
+			}
 
 			// Find all page entry points and create a map of the entry point to the hashed hoisted script.
 			// This is used when we render so that we can add the script to the head.

packages/astro/src/core/build/vite-plugin-ssr.ts

@@ -133,17 +133,22 @@ function buildManifest(
 		staticFiles.push(entryModules[PAGE_SCRIPT_ID]);
 	}
 
+	const bareBase = removeTrailingForwardSlash(removeLeadingForwardSlash(settings.config.base));
+	const joinBase = (pth: string) => (bareBase ? bareBase + '/' + pth : pth);
+
 	for (const pageData of eachPageData(internals)) {
 		const scripts: SerializedRouteInfo['scripts'] = [];
 		if (pageData.hoistedScript) {
-			scripts.unshift(pageData.hoistedScript);
+			scripts.unshift(Object.assign({}, pageData.hoistedScript, {
+				value: joinBase(pageData.hoistedScript.value)
+			}));
 		}
 		if (settings.scripts.some((script) => script.stage === 'page')) {
 			scripts.push({ type: 'external', value: entryModules[PAGE_SCRIPT_ID] });
 		}
 
-		const bareBase = removeTrailingForwardSlash(removeLeadingForwardSlash(settings.config.base));
+		
-		const links = sortedCSS(pageData).map((pth) => (bareBase ? bareBase + '/' + pth : pth));
+		const links = sortedCSS(pageData).map((pth) => joinBase(pth));
 
 		routes.push({
 			file: '',

packages/astro/src/core/config/schema.ts

@@ -323,11 +323,23 @@ export function createRelativeSchema(cmd: string, fileProtocolRoot: URL) {
 			config.build.client = new URL('./dist/client/', config.outDir);
 		}
 		const trimmedBase = trimSlashes(config.base);
-		if (trimmedBase.length && config.trailingSlash === 'never') {
+
-			config.base = prependForwardSlash(trimmedBase);
+		// If there is no base but there is a base for site config, warn.
-		} else {
+		const sitePathname = config.site && new URL(config.site).pathname;
-			config.base = prependForwardSlash(appendForwardSlash(trimmedBase));
+		if(!trimmedBase.length && sitePathname && sitePathname !== '/') {
+			config.base = sitePathname;
+			/* eslint-disable no-console */
+			console.warn(`The site configuration value includes a pathname of ${sitePathname} but there is no base configuration.
+			
+A future version of Astro will stop using the site pathname when producing <link> and <script> tags. Set your site's base with the base configuration.`)
 		}
+
+		if(trimmedBase.length && config.trailingSlash === 'never') {
+      config.base = prependForwardSlash(trimmedBase);
+    } else {
+      config.base = prependForwardSlash(appendForwardSlash(trimmedBase));
+    }
+
 		return config;
 	});
 

packages/astro/src/core/render/ssr-element.ts

@@ -3,34 +3,34 @@ import type { SSRElement } from '../../@types/astro';
 import npath from 'path-browserify';
 import { appendForwardSlash } from '../../core/path.js';
 
-function getRootPath(site?: string): string {
+function getRootPath(base?: string): string {
-	return appendForwardSlash(new URL(site || 'http://localhost/').pathname);
+	return appendForwardSlash(new URL(base || '/', 'http://localhost/').pathname);
 }
 
-function joinToRoot(href: string, site?: string): string {
+function joinToRoot(href: string, base?: string): string {
-	return npath.posix.join(getRootPath(site), href);
+	return npath.posix.join(getRootPath(base), href);
 }
 
-export function createLinkStylesheetElement(href: string, site?: string): SSRElement {
+export function createLinkStylesheetElement(href: string, base?: string): SSRElement {
 	return {
 		props: {
 			rel: 'stylesheet',
-			href: joinToRoot(href, site),
+			href: joinToRoot(href, base),
 		},
 		children: '',
 	};
 }
 
-export function createLinkStylesheetElementSet(hrefs: string[], site?: string) {
+export function createLinkStylesheetElementSet(hrefs: string[], base?: string) {
-	return new Set<SSRElement>(hrefs.map((href) => createLinkStylesheetElement(href, site)));
+	return new Set<SSRElement>(hrefs.map((href) => createLinkStylesheetElement(href, base)));
 }
 
 export function createModuleScriptElement(
 	script: { type: 'inline' | 'external'; value: string },
-	site?: string
+	base?: string
 ): SSRElement {
 	if (script.type === 'external') {
-		return createModuleScriptElementWithSrc(script.value, site);
+		return createModuleScriptElementWithSrc(script.value, base);
 	} else {
 		return {
 			props: {
@@ -60,7 +60,7 @@ export function createModuleScriptElementWithSrcSet(
 
 export function createModuleScriptsSet(
 	scripts: { type: 'inline' | 'external'; value: string }[],
-	site?: string
+	base?: string
 ): Set<SSRElement> {
-	return new Set<SSRElement>(scripts.map((script) => createModuleScriptElement(script, site)));
+	return new Set<SSRElement>(scripts.map((script) => createModuleScriptElement(script, base)));
 }

packages/astro/test/asset-url-base.test.js

@@ -0,0 +1,52 @@
+import { expect } from 'chai';
+import * as cheerio from 'cheerio';
+import { loadFixture } from './test-utils.js';
+
+describe('Asset URL resolution in build', () => {
+	/** @type {import('./test-utils').Fixture} */
+	let fixture;
+
+	describe('With site and base', async () => {
+		describe('with site', () => {
+			before(async () => {
+				fixture = await loadFixture({
+					root: './fixtures/asset-url-base/',
+					site: 'http://example.com/sub/path/'
+				});
+				await fixture.build();
+			});
+			
+			it('does not include the site\'s subpath', async () => {
+				const html = await fixture.readFile('/index.html');
+				const $ = cheerio.load(html);
+				const href = $('link[rel=stylesheet]').attr('href');
+				expect(href.startsWith('/sub/path/')).to.equal(false);
+			});
+		});
+
+		describe('with site and base', () => {
+			before(async () => {
+				fixture = await loadFixture({
+					root: './fixtures/asset-url-base/',
+					site: 'http://example.com/sub/path/',
+					base: '/another/base/'
+				});
+				await fixture.build();
+			});
+			
+			it('does not include the site\'s subpath', async () => {
+				const html = await fixture.readFile('/index.html');
+				const $ = cheerio.load(html);
+				const href = $('link[rel=stylesheet]').attr('href');
+				expect(href.startsWith('/sub/path/')).to.equal(false);
+			});
+
+			it('does include the base subpath', async () => {
+				const html = await fixture.readFile('/index.html');
+				const $ = cheerio.load(html);
+				const href = $('link[rel=stylesheet]').attr('href');
+				expect(href.startsWith('/another/base/')).to.equal(true);
+			});
+		});
+	});
+});

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

@@ -3,20 +3,18 @@ import * as cheerio from 'cheerio';
 import { loadFixture } from './test-utils.js';
 
 describe('Scripts (hoisted and not)', () => {
-	let fixture;
-	before(async () => {
-		fixture = await loadFixture({
-			root: './fixtures/astro-scripts/',
-			vite: {
-				build: {
-					assetsInlineLimit: 0,
-				},
-			},
-		});
-	});
-
 	describe('Build', () => {
+		/** @type {import('./test-utils').Fixture} */
+		let fixture;
 		before(async () => {
+			fixture = await loadFixture({
+				root: './fixtures/astro-scripts/',
+				vite: {
+					build: {
+						assetsInlineLimit: 0,
+					},
+				},
+			});
 			await fixture.build();
 		});
 
@@ -45,11 +43,8 @@ describe('Scripts (hoisted and not)', () => {
 			// test 1: Just one entry module
 			expect($el).to.have.lengthOf(1);
 
-			// test 2: attr removed
+			const src = $el.attr('src');
-			expect($el.attr('data-astro')).to.equal(undefined);
+			const inlineEntryJS = await fixture.readFile(src);
-
-			expect($el.attr('src')).to.equal(undefined);
-			const inlineEntryJS = $el.text();
 
 			// test 3: the JS exists
 			expect(inlineEntryJS).to.be.ok;
@@ -69,21 +64,6 @@ describe('Scripts (hoisted and not)', () => {
 			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);
@@ -122,12 +102,48 @@ describe('Scripts (hoisted and not)', () => {
 			// Imported styles + tailwind
 			expect($('link[rel=stylesheet]')).to.have.a.lengthOf(2);
 		});
+
+		describe('Inlining', () => {
+			/** @type {import('./test-utils').Fixture} */
+			let fixture;
+			before(async () => {
+				fixture = await loadFixture({
+					root: './fixtures/astro-scripts/',
+				});
+				await fixture.build();
+			});
+
+			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;
+			});
+		})
 	});
 
 	describe('Dev', () => {
+		/** @type {import('./test-utils').Fixture} */
+		let fixture;
 		/** @type {import('./test-utils').DevServer} */
 		let devServer;
 		before(async () => {
+			fixture = await loadFixture({
+				root: './fixtures/astro-scripts/',
+				vite: {
+					build: {
+						assetsInlineLimit: 0,
+					},
+				},
+			});
 			devServer = await fixture.startDevServer();
 		});
 

packages/astro/test/fixtures/asset-url-base/package.json

@@ -0,0 +1,11 @@
+{
+  "name": "@test/asset-url-base",
+  "version": "0.0.0",
+  "private": true,
+  "dependencies": {
+    "astro": "workspace:*"
+  },
+  "scripts": {
+    "dev": "astro dev"
+  }
+}

packages/astro/test/fixtures/asset-url-base/src/pages/index.astro

@@ -0,0 +1,13 @@
+<html>
+<head>
+	<title>Testing</title>
+	<style>
+		body {
+			background: blue;
+		}
+	</style>
+</head>
+<body>
+	<h1>Testing</h1>
+</body>
+</html>

packages/astro/test/fixtures/ssr-request/src/pages/request.astro

@@ -10,6 +10,9 @@ const origin = Astro.url.origin;
 			background: orangered;
 		}
 	</style>
+	<script>
+		console.log('hello world');
+	</script>
 </head>
 <body>
 <h1 id="origin">{origin}</h1>

packages/astro/test/ssr-request.test.js

@@ -13,6 +13,11 @@ describe('Using Astro.request in SSR', () => {
 			adapter: testAdapter(),
 			output: 'server',
 			base: '/subpath/',
+			vite: {
+				build: {
+					assetsInlineLimit: 0
+				}
+			}
 		});
 		await fixture.build();
 	});
@@ -51,6 +56,25 @@ describe('Using Astro.request in SSR', () => {
 		expect(css).to.not.be.an('undefined');
 	});
 
+	it('script assets have their base prefix', async () => {
+		const app = await fixture.loadTestAdapterApp();
+		let request = new Request('http://example.com/subpath/request');
+		let response = await app.render(request);
+		expect(response.status).to.equal(200);
+		const html = await response.text();
+		const $ = cheerioLoad(html);
+
+		const scriptSrc = $('script').attr('src');
+		expect(scriptSrc.startsWith('/subpath/')).to.equal(true);
+
+		request = new Request('http://example.com' + scriptSrc);
+		response = await app.render(request);
+
+		expect(response.status).to.equal(200);
+		const js = await response.text();
+		expect(js).to.not.be.an('undefined');
+	});
+
 	it('assets can be fetched', async () => {
 		const app = await fixture.loadTestAdapterApp();
 		const request = new Request('http://example.com/subpath/cars.json');

pnpm-lock.yaml

@@ -1150,6 +1150,12 @@ importers:
     dependencies:
       astro: link:../../..
 
+  packages/astro/test/fixtures/asset-url-base:
+    specifiers:
+      astro: workspace:*
+    dependencies:
+      astro: link:../../..
+
   packages/astro/test/fixtures/astro pages:
     specifiers:
       astro: workspace:*