Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make directRenderScript the default #11791

Merged
merged 4 commits into from
Aug 28, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions .changeset/five-jars-hear.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
---
'astro': patch
---

Updates Astro's default `<script>` rendering strategy and removes the `experimental.directRenderScript` option as this is now the default behavior: scripts are always rendered directly. This new strategy prevents scripts from being executed in pages where they are not used.

Scripts will directly render as declared in Astro files (including existing features like TypeScript, importing `node_modules`, and deduplicating scripts). You can also now conditionally render scripts in your Astro file.

However, this means scripts are no longer hoisted to the `<head>`, multiple scripts on a page are no longer bundled together, and the `<script>` tag may interfere with the CSS styling.

As this is a potentially breaking change to your script behavior, please review your `<script>` tags and ensure that they behave as expected.
4 changes: 2 additions & 2 deletions packages/astro/e2e/astro-component.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ test.describe('Astro component HMR', () => {
);
});

test('hoisted scripts', async ({ page, astro }) => {
test('Scripts', async ({ page, astro }) => {
const initialLog = page.waitForEvent(
'console',
(message) => message.text() === 'Hello, Astro!',
Expand All @@ -52,7 +52,7 @@ test.describe('Astro component HMR', () => {
(message) => message.text() === 'Hello, updated Astro!',
);

// Edit the hoisted script on the page
// Edit the script on the page
await astro.editFile('./src/pages/index.astro', (content) =>
content.replace('Hello, Astro!', 'Hello, updated Astro!'),
);
Expand Down
1 change: 0 additions & 1 deletion packages/astro/src/content/consts.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ export const ASSET_IMPORTS_VIRTUAL_ID = 'astro:asset-imports';
export const ASSET_IMPORTS_RESOLVED_STUB_ID = '\0' + ASSET_IMPORTS_VIRTUAL_ID;
export const LINKS_PLACEHOLDER = '@@ASTRO-LINKS@@';
export const STYLES_PLACEHOLDER = '@@ASTRO-STYLES@@';
export const SCRIPTS_PLACEHOLDER = '@@ASTRO-SCRIPTS@@';
export const IMAGE_IMPORT_PREFIX = '__ASTRO_IMAGE_';

export const CONTENT_FLAGS = [
Expand Down
66 changes: 4 additions & 62 deletions packages/astro/src/content/vite-plugin-content-assets.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,12 @@ import type { ModuleLoader } from '../core/module-loader/loader.js';
import { createViteLoader } from '../core/module-loader/vite.js';
import { joinPaths, prependForwardSlash } from '../core/path.js';
import type { AstroSettings } from '../types/astro.js';
import type { SSRElement } from '../types/public/internal.js';
import { getStylesForURL } from '../vite-plugin-astro-server/css.js';
import { getScriptsForURL } from '../vite-plugin-astro-server/scripts.js';
import {
CONTENT_IMAGE_FLAG,
CONTENT_RENDER_FLAG,
LINKS_PLACEHOLDER,
PROPAGATED_ASSET_FLAG,
SCRIPTS_PLACEHOLDER,
STYLES_PLACEHOLDER,
} from './consts.js';
import { hasContentFlag } from './utils.js';
Expand Down Expand Up @@ -69,7 +66,7 @@ export function astroContentAssetPropagationPlugin({
async transform(_, id, options) {
if (hasContentFlag(id, PROPAGATED_ASSET_FLAG)) {
const basePath = id.split('?')[0];
let stringifiedLinks: string, stringifiedStyles: string, stringifiedScripts: string;
let stringifiedLinks: string, stringifiedStyles: string;

// We can access the server in dev,
// so resolve collected styles and scripts here.
Expand All @@ -83,16 +80,6 @@ export function astroContentAssetPropagationPlugin({
crawledFiles: styleCrawledFiles,
} = await getStylesForURL(pathToFileURL(basePath), devModuleLoader);

// Add hoisted script tags, skip if direct rendering with `directRenderScript`
const { scripts: hoistedScripts, crawledFiles: scriptCrawledFiles } = settings.config
.experimental.directRenderScript
? { scripts: new Set<SSRElement>(), crawledFiles: new Set<string>() }
: await getScriptsForURL(
pathToFileURL(basePath),
settings.config.root,
devModuleLoader,
);

// Register files we crawled to be able to retrieve the rendered styles and scripts,
// as when they get updated, we need to re-transform ourselves.
// We also only watch files within the user source code, as changes in node_modules
Expand All @@ -102,22 +89,15 @@ export function astroContentAssetPropagationPlugin({
this.addWatchFile(file);
}
}
for (const file of scriptCrawledFiles) {
if (!file.includes('node_modules')) {
this.addWatchFile(file);
}
}

stringifiedLinks = JSON.stringify([...urls]);
stringifiedStyles = JSON.stringify(styles.map((s) => s.content));
stringifiedScripts = JSON.stringify([...hoistedScripts]);
} else {
// Otherwise, use placeholders to inject styles and scripts
// during the production bundle step.
// @see the `astro:content-build-plugin` below.
stringifiedLinks = JSON.stringify(LINKS_PLACEHOLDER);
stringifiedStyles = JSON.stringify(STYLES_PLACEHOLDER);
stringifiedScripts = JSON.stringify(SCRIPTS_PLACEHOLDER);
}

const code = `
Expand All @@ -126,8 +106,7 @@ export function astroContentAssetPropagationPlugin({
}
const collectedLinks = ${stringifiedLinks};
const collectedStyles = ${stringifiedStyles};
const collectedScripts = ${stringifiedScripts};
const defaultMod = { __astroPropagation: true, getMod, collectedLinks, collectedStyles, collectedScripts };
const defaultMod = { __astroPropagation: true, getMod, collectedLinks, collectedStyles, collectedScripts: [] };
export default defaultMod;
`;
// ^ Use a default export for tools like Markdoc
Expand All @@ -145,7 +124,7 @@ export function astroConfigBuildPlugin(
return {
targets: ['server'],
hooks: {
'build:post': ({ ssrOutputs, clientOutputs, mutate }) => {
'build:post': ({ ssrOutputs, mutate }) => {
const outputs = ssrOutputs.flatMap((o) => o.output);
const prependBase = (src: string) => {
const { assetsPrefix } = options.settings.config.build;
Expand All @@ -158,17 +137,12 @@ export function astroConfigBuildPlugin(
}
};
for (const chunk of outputs) {
if (
chunk.type === 'chunk' &&
(chunk.code.includes(LINKS_PLACEHOLDER) || chunk.code.includes(SCRIPTS_PLACEHOLDER))
) {
if (chunk.type === 'chunk' && chunk.code.includes(LINKS_PLACEHOLDER)) {
const entryStyles = new Set<string>();
const entryLinks = new Set<string>();
const entryScripts = new Set<string>();

for (const id of chunk.moduleIds) {
const _entryCss = internals.propagatedStylesMap.get(id);
const _entryScripts = internals.propagatedScriptsMap.get(id);
if (_entryCss) {
// TODO: Separating styles and links this way is not ideal. The `entryCss` list is order-sensitive
// and splitting them into two sets causes the order to be lost, because styles are rendered after
Expand All @@ -178,11 +152,6 @@ export function astroConfigBuildPlugin(
if (value.type === 'external') entryLinks.add(value.src);
}
}
if (_entryScripts) {
for (const value of _entryScripts) {
entryScripts.add(value);
}
}
}

let newCode = chunk.code;
Expand All @@ -202,33 +171,6 @@ export function astroConfigBuildPlugin(
} else {
newCode = newCode.replace(JSON.stringify(LINKS_PLACEHOLDER), '[]');
}
if (entryScripts.size) {
const entryFileNames = new Set<string>();
for (const output of clientOutputs) {
for (const clientChunk of output.output) {
if (clientChunk.type !== 'chunk') continue;
for (const [id] of Object.entries(clientChunk.modules)) {
if (entryScripts.has(id)) {
entryFileNames.add(clientChunk.fileName);
}
}
}
}
newCode = newCode.replace(
JSON.stringify(SCRIPTS_PLACEHOLDER),
JSON.stringify(
[...entryFileNames].map((src) => ({
props: {
src: prependBase(src),
type: 'module',
},
children: '',
})),
),
);
} else {
newCode = newCode.replace(JSON.stringify(SCRIPTS_PLACEHOLDER), '[]');
}
mutate(chunk, ['server'], newCode);
}
}
Expand Down
3 changes: 1 addition & 2 deletions packages/astro/src/core/build/generate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,6 @@ async function generatePage(
.reduce(mergeInlineCss, []);
// may be used in the future for handling rel=modulepreload, rel=icon, rel=manifest etc.
const linkIds: [] = [];
const scripts = pageData.hoistedScript ?? null;
if (!pageModulePromise) {
throw new Error(
`Unable to find the module for ${pageData.component}. This is unexpected and likely a bug in Astro, please report.`,
Expand All @@ -170,7 +169,7 @@ async function generatePage(
const generationOptions: Readonly<GeneratePathOptions> = {
pageData,
linkIds,
scripts,
scripts: null,
styles,
mod: pageModule,
};
Expand Down
63 changes: 5 additions & 58 deletions packages/astro/src/core/build/internal.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,10 @@ export interface BuildInternals {
*/
cssModuleToChunkIdMap: Map<string, string>;

// A mapping of hoisted script ids back to the exact hoisted scripts it references
hoistedScriptIdToHoistedMap: Map<string, Set<string>>;
// A mapping of hoisted script ids back to the pages which reference it
hoistedScriptIdToPagesMap: Map<string, Set<string>>;

/**
* Used by the `directRenderScript` option. If script is inlined, its id and
* inlined code is mapped here. The resolved id is an URL like "/_astro/something.js"
* but will no longer exist as the content is now inlined in this map.
* If script is inlined, its id and inlined code is mapped here. The resolved id is
* an URL like "/_astro/something.js" but will no longer exist as the content is now
* inlined in this map.
*/
inlinedScripts: Map<string, string>;

Expand Down Expand Up @@ -72,7 +67,7 @@ export interface BuildInternals {
*/
discoveredClientOnlyComponents: Map<string, string[]>;
/**
* A list of hoisted scripts that are discovered during the SSR build
* A list of scripts that are discovered during the SSR build.
* These will be used as the top-level entrypoints for the client build.
*/
discoveredScripts: Set<string>;
Expand All @@ -85,11 +80,6 @@ export interface BuildInternals {
* to a set of stylesheets that it uses.
*/
propagatedStylesMap: Map<string, Set<StylesheetAsset>>;
/**
* Map of propagated module ids (usually something like `/Users/...blog.mdx?astroPropagatedAssets`)
* to a set of hoisted scripts that it uses.
*/
propagatedScriptsMap: Map<string, Set<string>>;

// A list of all static files created during the build. Used for SSR.
staticFiles: Set<string>;
Expand All @@ -113,17 +103,9 @@ export interface BuildInternals {
* @returns {BuildInternals}
*/
export function createBuildInternals(): BuildInternals {
// These are for tracking hoisted script bundling
const hoistedScriptIdToHoistedMap = new Map<string, Set<string>>();

// This tracks hoistedScriptId => page components
const hoistedScriptIdToPagesMap = new Map<string, Set<string>>();

return {
cachedClientEntries: [],
cssModuleToChunkIdMap: new Map(),
hoistedScriptIdToHoistedMap,
hoistedScriptIdToPagesMap,
inlinedScripts: new Map(),
entrySpecifierToBundleMap: new Map<string, string>(),
pagesByKeys: new Map(),
Expand All @@ -132,7 +114,6 @@ export function createBuildInternals(): BuildInternals {
pagesByScriptId: new Map(),

propagatedStylesMap: new Map(),
propagatedScriptsMap: new Map(),

discoveredHydratedComponents: new Map(),
discoveredClientOnlyComponents: new Map(),
Expand Down Expand Up @@ -179,7 +160,7 @@ export function trackClientOnlyPageDatas(
}

/**
* Tracks scripts to the pages they are associated with. (experimental.directRenderScript)
* Tracks scripts to the pages they are associated with.
*/
export function trackScriptPageDatas(
internals: BuildInternals,
Expand Down Expand Up @@ -247,19 +228,6 @@ export function getPageData(
return undefined;
}

/**
* Get all pages datas from the build internals, using a specific component.
* @param internals Build Internals with all the pages
* @param component path to the component, used to identify related pages
*/
function getPagesDatasByComponent(internals: BuildInternals, component: string): PageBuildData[] {
const pageDatas: PageBuildData[] = [];
internals.pagesByKeys.forEach((pageData) => {
if (component === pageData.component) pageDatas.push(pageData);
});
return pageDatas;
}

// TODO: Should be removed in the future. (Astro 5?)
/**
* Map internals.pagesByKeys to a new map with the public key instead of the internal key.
Expand Down Expand Up @@ -371,24 +339,3 @@ export function mergeInlineCss(
acc.push(current);
return acc;
}

/**
* Get all pages data from the build internals, using a specific hoisted script id.
* @param internals Build Internals with all the pages
* @param id Hoisted script id, used to identify the pages using it
*/
export function getPageDatasByHoistedScriptId(
internals: BuildInternals,
id: string,
): PageBuildData[] {
const set = internals.hoistedScriptIdToPagesMap.get(id);
const pageDatas: PageBuildData[] = [];
if (set) {
for (const pageId of set) {
getPagesDatasByComponent(internals, pageId.slice(1)).forEach((pageData) => {
pageDatas.push(pageData);
});
}
}
return pageDatas;
}
2 changes: 0 additions & 2 deletions packages/astro/src/core/build/page-data.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ export function collectPagesData(opts: CollectPagesDataOptions): CollectPagesDat
route,
moduleSpecifier: '',
styles: [],
hoistedScript: undefined,
};

if (settings.config.output === 'static') {
Expand All @@ -60,7 +59,6 @@ export function collectPagesData(opts: CollectPagesDataOptions): CollectPagesDat
route,
moduleSpecifier: '',
styles: [],
hoistedScript: undefined,
};
}

Expand Down
19 changes: 8 additions & 11 deletions packages/astro/src/core/build/pipeline.ts
Original file line number Diff line number Diff line change
@@ -1,17 +1,18 @@
import { getOutputDirectory } from '../../prerender/utils.js';
import type { ComponentInstance } from '../../types/astro.js';
import type { RewritePayload } from '../../types/public/common.js';
import type { RouteData, SSRLoadedRenderer, SSRResult } from '../../types/public/internal.js';
import type {
RouteData,
SSRElement,
SSRLoadedRenderer,
SSRResult,
} from '../../types/public/internal.js';
import { BEFORE_HYDRATION_SCRIPT_ID, PAGE_SCRIPT_ID } from '../../vite-plugin-scripts/index.js';
import type { SSRManifest } from '../app/types.js';
import { routeIsFallback, routeIsRedirect } from '../redirects/helpers.js';
import { RedirectSinglePageBuiltModule } from '../redirects/index.js';
import { Pipeline } from '../render/index.js';
import {
createAssetLink,
createModuleScriptsSet,
createStylesheetElementSet,
} from '../render/ssr-element.js';
import { createAssetLink, createStylesheetElementSet } from '../render/ssr-element.js';
import { createDefaultRoutes } from '../routing/default.js';
import { findRouteToRewrite } from '../routing/rewrite.js';
import { isServerLikeOutput } from '../util.js';
Expand Down Expand Up @@ -153,11 +154,7 @@ export class BuildPipeline extends Pipeline {
} = this;
const links = new Set<never>();
const pageBuildData = getPageData(internals, routeData.route, routeData.component);
const scripts = createModuleScriptsSet(
pageBuildData?.hoistedScript ? [pageBuildData.hoistedScript] : [],
base,
assetsPrefix,
);
const scripts = new Set<SSRElement>();
const sortedCssAssets = pageBuildData?.styles
.sort(cssOrder)
.map(({ sheet }) => sheet)
Expand Down
Loading
Loading