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 hoisted script propagation in content collection pages #6119

Merged
merged 9 commits into from
Feb 6, 2023
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/shaggy-pianos-sleep.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'astro': patch
---

Fix hoisted script propagation in content collection pages
1 change: 0 additions & 1 deletion packages/astro/src/content/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ export { createContentTypesGenerator } from './types-generator.js';
export { contentObservable, getContentPaths, getDotAstroTypeReference } from './utils.js';
export {
astroContentAssetPropagationPlugin,
astroContentProdBundlePlugin,
} from './vite-plugin-content-assets.js';
export { astroContentServerPlugin } from './vite-plugin-content-server.js';
export { astroContentVirtualModPlugin } from './vite-plugin-content-virtual-mod.js';
116 changes: 86 additions & 30 deletions packages/astro/src/content/vite-plugin-content-assets.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { pathToFileURL } from 'url';
import npath from 'node:path';
import type { Plugin } from 'vite';
import { moduleIsTopLevelPage, walkParentInfos } from '../core/build/graph.js';
import { BuildInternals, getPageDataByViteID } from '../core/build/internal.js';
Expand All @@ -14,6 +15,8 @@ import {
SCRIPTS_PLACEHOLDER,
STYLES_PLACEHOLDER,
} from './consts.js';
import type { RollupOutput, OutputChunk, StaticBuildOptions } from '../core/build/types';
import { prependForwardSlash } from '../core/path.js';

function isPropagatedAsset(viteId: string): boolean {
const url = new URL(viteId, 'file://');
Expand Down Expand Up @@ -73,42 +76,95 @@ export function astroContentAssetPropagationPlugin({ mode }: { mode: string }):
};
}

export function astroContentProdBundlePlugin({ internals }: { internals: BuildInternals }): Plugin {
return {
name: 'astro:content-prod-bundle',
async generateBundle(_options, bundle) {
for (const [_, chunk] of Object.entries(bundle)) {
if (chunk.type === 'chunk' && chunk.code.includes(LINKS_PLACEHOLDER)) {
for (const id of Object.keys(chunk.modules)) {
for (const [pageInfo, depth, order] of walkParentInfos(id, this)) {
if (moduleIsTopLevelPage(pageInfo)) {
const pageViteID = pageInfo.id;
const pageData = getPageDataByViteID(internals, pageViteID);
if (!pageData) continue;
const entryCss = pageData.contentCollectionCss?.get(id);
if (!entryCss) continue;
chunk.code = chunk.code.replace(
JSON.stringify(LINKS_PLACEHOLDER),
JSON.stringify([...entryCss])
);
}
}
}
}
}
},
};
}

export function astroConfigBuildPlugin(internals: BuildInternals): AstroBuildPlugin {
export function astroConfigBuildPlugin(options: StaticBuildOptions, internals: BuildInternals): AstroBuildPlugin {
let ssrPluginContext: any = undefined;
return {
build: 'ssr',
hooks: {
'build:before': () => {
'build:before': ({ build }) => {
return {
vitePlugin: astroContentProdBundlePlugin({ internals }),
vitePlugin: {
name: 'astro:content-build-plugin',
generateBundle() {
if(build === 'ssr') {
ssrPluginContext = this;
}
}
},
};
},
'build:post': ({ ssrOutputs, clientOutputs, mutate }) => {
const outputs = ssrOutputs.flatMap(o => o.output);
for (const chunk of outputs) {
if (
chunk.type === 'chunk' &&
(chunk.code.includes(LINKS_PLACEHOLDER) || chunk.code.includes(SCRIPTS_PLACEHOLDER))
) {
let entryCSS = new Set<string>();
let entryScripts = new Set<string>();

for (const id of Object.keys(chunk.modules)) {
for (const [pageInfo] of walkParentInfos(id, ssrPluginContext)) {
if (moduleIsTopLevelPage(pageInfo)) {
const pageViteID = pageInfo.id;
const pageData = getPageDataByViteID(internals, pageViteID);
if (!pageData) continue;

const _entryCss = pageData.propagatedStyles?.get(id);
const _entryScripts = pageData.propagatedScripts?.get(id);
if(_entryCss) {
for(const value of _entryCss) {
entryCSS.add(value);
}
}
if(_entryScripts) {
for(const value of _entryScripts) {
entryScripts.add(value);
}
}
}
}
}

let newCode = chunk.code;
if (entryCSS.size) {
newCode = newCode.replace(
JSON.stringify(LINKS_PLACEHOLDER),
JSON.stringify([...entryCSS])
);
}
if (entryScripts.size) {
const entryFileNames = new Set<string>();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love that we can look these up now 👏 Way simpler than scary naming conventions!

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: prependForwardSlash(npath.posix.join(
options.settings.config.base,
src
)),
type: 'module'
},
children: '',
}))
)
);
}
mutate(chunk, 'server', newCode);
}
}
}
},
};
}
2 changes: 1 addition & 1 deletion packages/astro/src/core/build/graph.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ export function moduleIsTopLevelPage(info: ModuleInfo): boolean {
// This could be a .astro page, a .markdown or a .md (or really any file extension for markdown files) page.
export function* getTopLevelPages(
id: string,
ctx: { getModuleInfo: GetModuleInfo }
ctx: { getModuleInfo: GetModuleInfo },
): Generator<[ModuleInfo, number, number], void, unknown> {
for (const res of walkParentInfos(id, ctx)) {
if (moduleIsTopLevelPage(res[0])) {
Expand Down
6 changes: 4 additions & 2 deletions packages/astro/src/core/build/page-data.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,8 @@ export async function collectPagesData(
route,
moduleSpecifier: '',
css: new Map(),
contentCollectionCss: new Map(),
propagatedStyles: new Map(),
propagatedScripts: new Map(),
hoistedScript: undefined,
};

Expand All @@ -76,7 +77,8 @@ export async function collectPagesData(
route,
moduleSpecifier: '',
css: new Map(),
contentCollectionCss: new Map(),
propagatedStyles: new Map(),
propagatedScripts: new Map(),
hoistedScript: undefined,
};
}
Expand Down
2 changes: 1 addition & 1 deletion packages/astro/src/core/build/plugins/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ export function registerAllPlugins({ internals, options, register }: AstroBuildP
register(pluginPages(options, internals));
register(pluginCSS(options, internals));
register(pluginPrerender(options, internals));
register(astroConfigBuildPlugin(internals));
register(astroConfigBuildPlugin(options, internals));
register(pluginHoistedScripts(options, internals));
register(pluginSSR(options, internals));
}
76 changes: 62 additions & 14 deletions packages/astro/src/core/build/plugins/plugin-analyzer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,29 @@ import type { PluginMetadata as AstroPluginMetadata } from '../../../vite-plugin
import type { BuildInternals } from '../internal.js';
import type { AstroBuildPlugin } from '../plugin.js';

import { prependForwardSlash } from '../../path.js';
import { getTopLevelPages } from '../graph.js';
import { prependForwardSlash } from '../../../core/path.js';
import { getTopLevelPages, moduleIsTopLevelPage, walkParentInfos } from '../graph.js';
import { getPageDataByViteID, trackClientOnlyPageDatas } from '../internal.js';
import { PROPAGATED_ASSET_FLAG } from '../../../content/consts.js';

function isPropagatedAsset(id: string) {
try {
return new URL('file://' + id).searchParams.has(PROPAGATED_ASSET_FLAG)
} catch {
return false;
}
}

export function vitePluginAnalyzer(internals: BuildInternals): VitePlugin {
function hoistedScriptScanner() {
const uniqueHoistedIds = new Map<string, string>();
const pageScripts = new Map<string, Set<string>>();
const pageScripts = new Map<
string,
{
hoistedSet: Set<string>;
propagatedMapByImporter: Map<string, Set<string>>;
}
>();

return {
scan(this: PluginContext, scripts: AstroPluginMetadata['astro']['scripts'], from: string) {
Expand All @@ -22,28 +37,51 @@ export function vitePluginAnalyzer(internals: BuildInternals): VitePlugin {
}

if (hoistedScripts.size) {
for (const [pageInfo] of getTopLevelPages(from, this)) {
const pageId = pageInfo.id;
for (const hid of hoistedScripts) {
if (pageScripts.has(pageId)) {
pageScripts.get(pageId)?.add(hid);
} else {
pageScripts.set(pageId, new Set([hid]));
for (const [parentInfo] of walkParentInfos(from, this, function until(importer) {
return isPropagatedAsset(importer);
})) {
if (isPropagatedAsset(parentInfo.id)) {
for (const [nestedParentInfo] of walkParentInfos(from, this)) {
if (moduleIsTopLevelPage(nestedParentInfo)) {
for (const hid of hoistedScripts) {
if (!pageScripts.has(nestedParentInfo.id)) {
pageScripts.set(nestedParentInfo.id, {
hoistedSet: new Set(),
propagatedMapByImporter: new Map(),
});
}
const entry = pageScripts.get(nestedParentInfo.id)!;
if (!entry.propagatedMapByImporter.has(parentInfo.id)) {
entry.propagatedMapByImporter.set(parentInfo.id, new Set());
}
entry.propagatedMapByImporter.get(parentInfo.id)!.add(hid);
}
}
}
} else if (moduleIsTopLevelPage(parentInfo)) {
for (const hid of hoistedScripts) {
if (!pageScripts.has(parentInfo.id)) {
pageScripts.set(parentInfo.id, {
hoistedSet: new Set(),
propagatedMapByImporter: new Map(),
});
}
pageScripts.get(parentInfo.id)?.hoistedSet.add(hid);
}
}
}
}
},

finalize() {
for (const [pageId, hoistedScripts] of pageScripts) {
for (const [pageId, { hoistedSet, propagatedMapByImporter }] of pageScripts) {
const pageData = getPageDataByViteID(internals, pageId);
if (!pageData) continue;

const { component } = pageData;
const astroModuleId = prependForwardSlash(component);

const uniqueHoistedId = JSON.stringify(Array.from(hoistedScripts).sort());
const uniqueHoistedId = JSON.stringify(Array.from(hoistedSet).sort());
let moduleId: string;

// If we're already tracking this set of hoisted scripts, get the unique id
Expand All @@ -56,13 +94,23 @@ export function vitePluginAnalyzer(internals: BuildInternals): VitePlugin {
}
internals.discoveredScripts.add(moduleId);

pageData.propagatedScripts = propagatedMapByImporter;

// Add propagated scripts to client build,
// but DON'T add to pages -> hoisted script map.
for (const propagatedScripts of propagatedMapByImporter.values()) {
for (const propagatedScript of propagatedScripts) {
internals.discoveredScripts.add(propagatedScript);
}
}

// Make sure to track that this page uses this set of hoisted scripts
if (internals.hoistedScriptIdToPagesMap.has(moduleId)) {
const pages = internals.hoistedScriptIdToPagesMap.get(moduleId);
pages!.add(astroModuleId);
} else {
internals.hoistedScriptIdToPagesMap.set(moduleId, new Set([astroModuleId]));
internals.hoistedScriptIdToHoistedMap.set(moduleId, hoistedScripts);
internals.hoistedScriptIdToHoistedMap.set(moduleId, hoistedSet);
}
}
},
Expand Down Expand Up @@ -132,7 +180,7 @@ export function pluginAnalyzer(internals: BuildInternals): AstroBuildPlugin {
return {
vitePlugin: vitePluginAnalyzer(internals),
};
},
}
},
};
}
4 changes: 2 additions & 2 deletions packages/astro/src/core/build/plugins/plugin-css.ts
Original file line number Diff line number Diff line change
Expand Up @@ -176,8 +176,8 @@ export function rollupPluginAstroBuildCSS(options: PluginOptions): VitePlugin[]
if (pageData) {
for (const css of meta.importedCss) {
const existingCss =
pageData.contentCollectionCss.get(pageInfo.id) ?? new Set();
pageData.contentCollectionCss.set(
pageData.propagatedStyles.get(pageInfo.id) ?? new Set();
pageData.propagatedStyles.set(
pageInfo.id,
new Set([...existingCss, css])
);
Expand Down
5 changes: 4 additions & 1 deletion packages/astro/src/core/build/static-build.ts
Original file line number Diff line number Diff line change
Expand Up @@ -245,9 +245,12 @@ async function runPostBuildHooks(
clientReturn: Awaited<ReturnType<typeof clientBuild>>
) {
const mutations = await container.runPostHook(ssrReturn, clientReturn);
const config = container.options.settings.config;
const buildConfig = container.options.settings.config.build;
for (const [fileName, mutation] of mutations) {
const root = mutation.build === 'server' ? buildConfig.server : buildConfig.client;
const root = config.output === 'server' ?
mutation.build === 'server' ? buildConfig.server : buildConfig.client :
config.outDir;
const fileURL = new URL(fileName, root);
await fs.promises.mkdir(new URL('./', fileURL), { recursive: true });
await fs.promises.writeFile(fileURL, mutation.code, 'utf-8');
Expand Down
3 changes: 2 additions & 1 deletion packages/astro/src/core/build/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@ export interface PageBuildData {
route: RouteData;
moduleSpecifier: string;
css: Map<string, { depth: number; order: number }>;
contentCollectionCss: Map<string, Set<string>>;
propagatedStyles: Map<string, Set<string>>;
propagatedScripts: Map<string, Set<string>>;
hoistedScript: { type: 'inline' | 'external'; value: string } | undefined;
}
export type AllPagesData = Record<ComponentPath, PageBuildData>;
Expand Down
6 changes: 2 additions & 4 deletions packages/astro/test/content-collections-render.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ describe('Content Collections - render()', () => {
// Includes hoisted script
expect(
[...allScripts].find((script) =>
$(script).text().includes('document.querySelector("#update-me")')
$(script).attr('src')?.includes('WithScripts')
),
'`WithScripts.astro` hoisted script missing from head.'
).to.not.be.undefined;
Expand All @@ -55,9 +55,7 @@ describe('Content Collections - render()', () => {
expect($('script[data-is-inline]')).to.have.a.lengthOf(1);
});

// TODO: Script bleed isn't solved for prod builds.
// Tackling in separate PR.
it.skip('Excludes component scripts for non-rendered entries', async () => {
it('Excludes component scripts for non-rendered entries', async () => {
const html = await fixture.readFile('/index.html');
const $ = cheerio.load(html);

Expand Down