Skip to content

Commit

Permalink
Fix hoisted script propagation in content collection pages (#6119)
Browse files Browse the repository at this point in the history 
* chore: unskip

* chore: stray console logs

* chore: clarify analyzer comments

* wip: store propagated scripts separately

* Implement getting hoisted scripts for propagation

* Add changeset

* fix

* oops

* update based on code review

---------

Co-authored-by: bholmesdev <hey@bholmes.dev>
  • Loading branch information
@matthewp @bholmesdev
matthewp and bholmesdev committed Feb 6, 2023
1 parent 4595dd6 commit 2189170
Show file tree
Hide file tree
Showing 11 changed files with 169 additions and 57 deletions.
5 changes: 5 additions & 0 deletions .changeset/shaggy-pianos-sleep.md
View file
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
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ export { createContentTypesGenerator } from './types-generator.js';
export { contentObservable, getContentPaths, getDotAstroTypeReference } from './utils.js';
export {
astroContentAssetPropagationPlugin,
astroContentProdBundlePlugin,
} from './vite-plugin-content-assets.js';
export { astroContentImportPlugin } from './vite-plugin-content-imports.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
View file
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>();
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
View file
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
View file
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
View file
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
View file
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
View file
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
View file
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
View file
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
View file
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

0 comments on commit 2189170

Please sign in to comment.