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

Fix CSS handling for experimental.directRenderScript #11026

Merged
merged 1 commit into from
May 13, 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
5 changes: 5 additions & 0 deletions .changeset/clever-ads-scream.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"astro": patch
---

Skips rendering script tags if it's inlined and empty when `experimental.directRenderScript` is enabled
5 changes: 5 additions & 0 deletions .changeset/rich-melons-worry.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"astro": patch
---

Fixes CSS handling if imported in a script tag in an Astro file when `experimental.directRenderScript` is enabled
26 changes: 26 additions & 0 deletions packages/astro/src/core/build/internal.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,11 @@ export interface BuildInternals {
*/
pagesByClientOnly: Map<string, Set<PageBuildData>>;

/**
* A map for page-specific information by a script in an Astro file
*/
pagesByScriptId: Map<string, Set<PageBuildData>>;

/**
* A map of hydrated components to export names that are discovered during the SSR build.
* These will be used as the top-level entrypoints for the client build.
Expand Down Expand Up @@ -133,6 +138,7 @@ export function createBuildInternals(): BuildInternals {
pageOptionsByPage: new Map(),
pagesByViteID: new Map(),
pagesByClientOnly: new Map(),
pagesByScriptId: new Map(),

propagatedStylesMap: new Map(),
propagatedScriptsMap: new Map(),
Expand Down Expand Up @@ -181,6 +187,26 @@ export function trackClientOnlyPageDatas(
}
}

/**
* Tracks scripts to the pages they are associated with. (experimental.directRenderScript)
*/
export function trackScriptPageDatas(
internals: BuildInternals,
pageData: PageBuildData,
scriptIds: string[]
) {
for (const scriptId of scriptIds) {
let pageDataSet: Set<PageBuildData>;
if (internals.pagesByScriptId.has(scriptId)) {
pageDataSet = internals.pagesByScriptId.get(scriptId)!;
} else {
pageDataSet = new Set<PageBuildData>();
internals.pagesByScriptId.set(scriptId, pageDataSet);
}
pageDataSet.add(pageData);
}
}

export function* getPageDatasByChunk(
internals: BuildInternals,
chunk: Rollup.RenderedChunk
Expand Down
24 changes: 20 additions & 4 deletions packages/astro/src/core/build/plugins/plugin-analyzer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,11 @@ import {
getTopLevelPageModuleInfos,
moduleIsTopLevelPage,
} from '../graph.js';
import { getPageDataByViteID, trackClientOnlyPageDatas } from '../internal.js';
import {
getPageDataByViteID,
trackClientOnlyPageDatas,
trackScriptPageDatas,
} from '../internal.js';
import type { StaticBuildOptions } from '../types.js';

function isPropagatedAsset(id: string) {
Expand Down Expand Up @@ -171,9 +175,21 @@ export function vitePluginAnalyzer(
// each script module is its own entrypoint, so we directly assign each script modules to
// `discoveredScripts` here, which will eventually be passed as inputs of the client build.
if (options.settings.config.experimental.directRenderScript && astro.scripts.length) {
for (let i = 0; i < astro.scripts.length; i++) {
const hid = `${id.replace('/@fs', '')}?astro&type=script&index=${i}&lang.ts`;
internals.discoveredScripts.add(hid);
const scriptIds = astro.scripts.map(
(_, i) => `${id.replace('/@fs', '')}?astro&type=script&index=${i}&lang.ts`
);

// Assign as entrypoints for the client bundle
for (const scriptId of scriptIds) {
internals.discoveredScripts.add(scriptId);
}

// The script may import CSS, so we also have to track the pages that use this script
for (const pageInfo of getTopLevelPageModuleInfos(id, this)) {
const newPageData = getPageDataByViteID(internals, pageInfo.id);
if (!newPageData) continue;

trackScriptPageDatas(internals, newPageData, scriptIds);
}
}
}
Expand Down
21 changes: 15 additions & 6 deletions packages/astro/src/core/build/plugins/plugin-css.ts
Original file line number Diff line number Diff line change
Expand Up @@ -145,12 +145,21 @@ function rollupPluginAstroBuildCSS(options: PluginOptions): VitePlugin[] {
if (pageData) {
appendCSSToPage(pageData, meta, pagesToCss, depth, order);
}
} else if (
options.target === 'client' &&
internals.hoistedScriptIdToPagesMap.has(pageInfo.id)
) {
for (const pageData of getPageDatasByHoistedScriptId(internals, pageInfo.id)) {
appendCSSToPage(pageData, meta, pagesToCss, -1, order);
} else if (options.target === 'client') {
// For scripts or hoisted scripts, walk parents until you find a page, and add the CSS to that page.
if (buildOptions.settings.config.experimental.directRenderScript) {
const pageDatas = internals.pagesByScriptId.get(pageInfo.id)!;
if (pageDatas) {
for (const pageData of pageDatas) {
appendCSSToPage(pageData, meta, pagesToCss, -1, order);
}
}
} else {
if (internals.hoistedScriptIdToPagesMap.has(pageInfo.id)) {
for (const pageData of getPageDatasByHoistedScriptId(internals, pageInfo.id)) {
appendCSSToPage(pageData, meta, pagesToCss, -1, order);
}
}
}
}
}
Expand Down
9 changes: 7 additions & 2 deletions packages/astro/src/runtime/server/render/script.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,13 @@ export async function renderScript(result: SSRResult, id: string) {
result._metadata.renderedScripts.add(id);

const inlined = result.inlinedScripts.get(id);
if (inlined) {
return markHTMLString(`<script type="module">${inlined}</script>`);
if (inlined != null) {
// The inlined script may actually be empty, so skip rendering it altogether if so
if (inlined) {
return markHTMLString(`<script type="module">${inlined}</script>`);
} else {
return '';
}
}

const resolved = await result.resolve(id);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
<html lang="en">
<head>
<script>
import "../styles/script-import-style.css";
</script>
</head>
<body>
<h1>Astro</h1>
</body>
</html>
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
h1 {
background-color: tomato;
}
10 changes: 10 additions & 0 deletions packages/astro/test/hoisted-imports.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -87,5 +87,15 @@ describe('Hoisted Imports', () => {
assert.ok(scripts[0].attribs.src);
assert.ok(scripts[1].attribs.src);
});

it('renders styles if imported from the script', async () => {
const html = await fixture.readFile('/script-import-style/index.html');
const $ = cheerio.load(html);
const styles = $('style');
assert.equal(styles.length, 1);
// There should be no script because it's empty (contains only CSS import)
const scripts = $('scripts');
assert.equal(scripts.length, 0);
});
});
});
Loading