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

[Markdoc] Fix global asset bleed #6758

Merged
merged 24 commits into from May 22, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
d26721b
wip: propagatedAssets flag per-component
bholmesdev Apr 3, 2023
b4bf54a
Propagate in TreeNode
matthewp Apr 4, 2023
535bf98
fix: remove unused inject comment
bholmesdev Apr 5, 2023
3a34cb2
feat: make asset propagation an integration opt-in
bholmesdev Apr 5, 2023
087e676
fix: remove crawlGraph stopper
bholmesdev Apr 5, 2023
16481a4
wip: logs to understand what's happening
bholmesdev Apr 5, 2023
4dd3dd5
SSR mdoc files in dev
matthewp Apr 25, 2023
1f05fdb
feat: add astroPropagatedAssets flag with vite
bholmesdev Apr 26, 2023
ec1a315
chore: remove console logs
bholmesdev Apr 26, 2023
df3e433
chore: cleanup hasContentFlag
bholmesdev Apr 26, 2023
fbd2801
fix: set handlePropagation default for legacy integrations
bholmesdev Apr 26, 2023
b90eebb
chore: changeset
bholmesdev Apr 26, 2023
9881738
temp: silence acorn type error
bholmesdev Apr 26, 2023
5b8d2b2
chore: revert pnpm-lock changes
bholmesdev Apr 26, 2023
4b18f42
fix: check correct flag
bholmesdev Apr 27, 2023
39e657f
Merge branch 'main' into fix/markdoc-global-assets
matthewp May 17, 2023
303792c
We need to handle propagation on markdown because of layouts
matthewp May 18, 2023
04daea0
Remove use of renderStyleElement
matthewp May 18, 2023
7c37771
Fix heading tests
matthewp May 18, 2023
e642caf
Merge branch 'main' into fix/markdoc-global-assets
matthewp May 19, 2023
568aed8
Fix merge conflict
matthewp May 19, 2023
61bf92e
typeof function
matthewp May 19, 2023
0e94ed4
Switch the check
matthewp May 19, 2023
96e470b
Add comment on injection detection regexp
matthewp May 22, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/polite-knives-greet.md
@@ -0,0 +1,7 @@
---
'astro': patch
'@astrojs/markdoc': patch
'@astrojs/mdx': patch
---

Improve style and script handling across content collection files. This addresses style bleed present in `@astrojs/markdoc` v0.1.0
9 changes: 0 additions & 9 deletions examples/with-markdoc/src/content/config.ts

This file was deleted.

6 changes: 6 additions & 0 deletions packages/astro/src/@types/astro.ts
Expand Up @@ -1275,6 +1275,12 @@ export interface ContentEntryType {
}
): rollup.LoadResult | Promise<rollup.LoadResult>;
contentModuleTypes?: string;
/**
* Handle asset propagation for rendered content to avoid bleed.
* Ex. MDX content can import styles and scripts, so `handlePropagation` should be true.
* @default true
*/
handlePropagation?: boolean;
}

type GetContentEntryInfoReturnType = {
Expand Down
4 changes: 3 additions & 1 deletion packages/astro/src/content/consts.ts
@@ -1,11 +1,13 @@
export const PROPAGATED_ASSET_FLAG = 'astroPropagatedAssets';
export const CONTENT_RENDER_FLAG = 'astroRenderContent';
export const CONTENT_FLAG = 'astroContentCollectionEntry';
export const DATA_FLAG = 'astroDataCollectionEntry';
export const CONTENT_FLAGS = [CONTENT_FLAG, DATA_FLAG, PROPAGATED_ASSET_FLAG] as const;

export const VIRTUAL_MODULE_ID = 'astro:content';
export const LINKS_PLACEHOLDER = '@@ASTRO-LINKS@@';
export const STYLES_PLACEHOLDER = '@@ASTRO-STYLES@@';
export const SCRIPTS_PLACEHOLDER = '@@ASTRO-SCRIPTS@@';

export const CONTENT_FLAGS = [CONTENT_FLAG, CONTENT_RENDER_FLAG, DATA_FLAG, PROPAGATED_ASSET_FLAG] as const;

export const CONTENT_TYPES_FILE = 'types.d.ts';
124 changes: 72 additions & 52 deletions packages/astro/src/content/runtime.ts
Expand Up @@ -270,62 +270,82 @@ async function render({
const baseMod = await renderEntryImport();
if (baseMod == null || typeof baseMod !== 'object') throw UnexpectedRenderError;

const { collectedStyles, collectedLinks, collectedScripts, getMod } = baseMod;
if (typeof getMod !== 'function') throw UnexpectedRenderError;
const mod = await getMod();
if (mod == null || typeof mod !== 'object') throw UnexpectedRenderError;
if (
baseMod.default != null &&
typeof baseMod.default === 'object' &&
baseMod.default.__astroPropagation === true
) {
const { collectedStyles, collectedLinks, collectedScripts, getMod } = baseMod.default;
if (typeof getMod !== 'function') throw UnexpectedRenderError;
const propagationMod = await getMod();
if (propagationMod == null || typeof propagationMod !== 'object') throw UnexpectedRenderError;

const Content = createComponent({
factory(result, baseProps, slots) {
let styles = '',
links = '',
scripts = '';
if (Array.isArray(collectedStyles)) {
styles = collectedStyles
.map((style: any) => {
return renderUniqueStylesheet(result, {
type: 'inline',
content: style,
});
})
.join('');
}
if (Array.isArray(collectedLinks)) {
links = collectedLinks
.map((link: any) => {
return renderUniqueStylesheet(result, {
type: 'external',
src: prependForwardSlash(link),
});
})
.join('');
}
if (Array.isArray(collectedScripts)) {
scripts = collectedScripts.map((script: any) => renderScriptElement(script)).join('');
}
const Content = createComponent({
factory(result, baseProps, slots) {
let styles = '',
links = '',
scripts = '';
if (Array.isArray(collectedStyles)) {
styles = collectedStyles
.map((style: any) => {
return renderUniqueStylesheet(result, {
type: 'inline',
content: style,
});
})
.join('');
}
if (Array.isArray(collectedLinks)) {
links = collectedLinks
.map((link: any) => {
return renderUniqueStylesheet(result, {
type: 'external',
src: prependForwardSlash(link),
});
})
.join('');
}
if (Array.isArray(collectedScripts)) {
scripts = collectedScripts.map((script: any) => renderScriptElement(script)).join('');
}

let props = baseProps;
// Auto-apply MDX components export
if (id.endsWith('mdx')) {
props = {
components: mod.components ?? {},
...baseProps,
};
}
let props = baseProps;
// Auto-apply MDX components export
if (id.endsWith('mdx')) {
props = {
components: propagationMod.components ?? {},
...baseProps,
};
}

return createHeadAndContent(
unescapeHTML(styles + links + scripts) as any,
renderTemplate`${renderComponent(result, 'Content', mod.Content, props, slots)}`
);
},
propagation: 'self',
});
return createHeadAndContent(
unescapeHTML(styles + links + scripts) as any,
renderTemplate`${renderComponent(
result,
'Content',
propagationMod.Content,
props,
slots
)}`
);
},
propagation: 'self',
});

return {
Content,
headings: mod.getHeadings?.() ?? [],
remarkPluginFrontmatter: mod.frontmatter ?? {},
};
return {
Content,
headings: propagationMod.getHeadings?.() ?? [],
remarkPluginFrontmatter: propagationMod.frontmatter ?? {},
};
} else if (baseMod.Content && typeof baseMod.Content === 'function') {
return {
Content: baseMod.Content,
headings: baseMod.getHeadings?.() ?? [],
remarkPluginFrontmatter: baseMod.frontmatter ?? {},
};
} else {
throw UnexpectedRenderError;
}
}

export function createReference({ lookupMap }: { lookupMap: ContentLookupMap }) {
Expand Down
2 changes: 1 addition & 1 deletion packages/astro/src/content/template/virtual-mod.mjs
Expand Up @@ -46,7 +46,7 @@ function createGlobLookup(glob) {
}

const renderEntryGlob = import.meta.glob('@@RENDER_ENTRY_GLOB_PATH@@', {
query: { astroPropagatedAssets: true },
query: { astroRenderContent: true },
});
const collectionToRenderEntryMap = createCollectionToGlobResultMap({
globResult: renderEntryGlob,
Expand Down
3 changes: 2 additions & 1 deletion packages/astro/src/content/utils.ts
Expand Up @@ -14,6 +14,7 @@ import type {
} from '../@types/astro.js';
import { VALID_INPUT_FORMATS } from '../assets/consts.js';
import { AstroError, AstroErrorData } from '../core/errors/index.js';

import { formatYAMLException, isYAMLException } from '../core/errors/utils.js';
import { CONTENT_FLAGS, CONTENT_TYPES_FILE } from './consts.js';
import { errorMap } from './error-map.js';
Expand Down Expand Up @@ -328,7 +329,7 @@ export function parseFrontmatter(fileContents: string, filePath: string) {
*/
export const globalContentConfigObserver = contentObservable({ status: 'init' });

export function hasContentFlag(viteId: string, flag: (typeof CONTENT_FLAGS)[number]) {
export function hasContentFlag(viteId: string, flag: (typeof CONTENT_FLAGS)[number]): boolean {
const flags = new URLSearchParams(viteId.split('?')[1] ?? '');
return flags.has(flag);
}
Expand Down
43 changes: 31 additions & 12 deletions packages/astro/src/content/vite-plugin-content-assets.ts
@@ -1,4 +1,5 @@
import { pathToFileURL } from 'url';
import { extname } from 'node:path';
import { pathToFileURL } from 'node:url';
import type { Plugin } from 'vite';
import type { AstroSettings } from '../@types/astro.js';
import { moduleIsTopLevelPage, walkParentInfos } from '../core/build/graph.js';
Expand All @@ -11,16 +12,13 @@ import { joinPaths, prependForwardSlash } from '../core/path.js';
import { getStylesForURL } from '../core/render/dev/css.js';
import { getScriptsForURL } from '../core/render/dev/scripts.js';
import {
CONTENT_RENDER_FLAG,
LINKS_PLACEHOLDER,
PROPAGATED_ASSET_FLAG,
SCRIPTS_PLACEHOLDER,
STYLES_PLACEHOLDER,
} from './consts.js';

function isPropagatedAsset(viteId: string) {
const flags = new URLSearchParams(viteId.split('?')[1]);
return flags.has(PROPAGATED_ASSET_FLAG);
}
import { hasContentFlag } from './utils.js';

export function astroContentAssetPropagationPlugin({
mode,
Expand All @@ -32,13 +30,31 @@ export function astroContentAssetPropagationPlugin({
let devModuleLoader: ModuleLoader;
return {
name: 'astro:content-asset-propagation',
enforce: 'pre',
async resolveId(id, importer, opts) {
if (hasContentFlag(id, CONTENT_RENDER_FLAG)) {
const base = id.split('?')[0];

for (const { extensions, handlePropagation = true } of settings.contentEntryTypes) {
if (handlePropagation && extensions.includes(extname(base))) {
return this.resolve(`${base}?${PROPAGATED_ASSET_FLAG}`, importer, {
skipSelf: true,
...opts,
});
}
}
// Resolve to the base id (no content flags)
// if Astro doesn't need to handle propagation.
return this.resolve(base, importer, { skipSelf: true, ...opts });
}
},
configureServer(server) {
if (mode === 'dev') {
devModuleLoader = createViteLoader(server);
}
},
async transform(_, id, options) {
if (isPropagatedAsset(id)) {
if (hasContentFlag(id, PROPAGATED_ASSET_FLAG)) {
const basePath = id.split('?')[0];
let stringifiedLinks: string, stringifiedStyles: string, stringifiedScripts: string;

Expand Down Expand Up @@ -73,14 +89,17 @@ export function astroContentAssetPropagationPlugin({
}

const code = `
export async function getMod() {
async function getMod() {
return import(${JSON.stringify(basePath)});
}
export const collectedLinks = ${stringifiedLinks};
export const collectedStyles = ${stringifiedStyles};
export const collectedScripts = ${stringifiedScripts};
const collectedLinks = ${stringifiedLinks};
const collectedStyles = ${stringifiedStyles};
const collectedScripts = ${stringifiedScripts};
const defaultMod = { __astroPropagation: true, getMod, collectedLinks, collectedStyles, collectedScripts };
export default defaultMod;
`;

// ^ Use a default export for tools like Markdoc
// to catch the `__astroPropagation` identifier
return { code, map: { mappings: '' } };
}
},
Expand Down
18 changes: 7 additions & 11 deletions packages/astro/src/core/render/dev/vite.ts
@@ -1,7 +1,6 @@
import type { ModuleLoader, ModuleNode } from '../../module-loader/index';

import npath from 'path';
import { PROPAGATED_ASSET_FLAG } from '../../../content/consts.js';
import { SUPPORTED_MARKDOWN_FILE_EXTENSIONS } from '../../constants.js';
import { unwrapId } from '../../util.js';
import { isCSSRequest } from './util.js';
Expand All @@ -10,9 +9,10 @@ import { isCSSRequest } from './util.js';
* List of file extensions signalling we can (and should) SSR ahead-of-time
* See usage below
*/
const fileExtensionsToSSR = new Set(['.astro', ...SUPPORTED_MARKDOWN_FILE_EXTENSIONS]);
const fileExtensionsToSSR = new Set(['.astro', '.mdoc', ...SUPPORTED_MARKDOWN_FILE_EXTENSIONS]);
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason we're hardcoding .mdoc here?

Copy link
Contributor

Choose a reason for hiding this comment

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

These are modules that we need to ensure are imported before we do rendering in dev. We need to do this so that we can discover which CSS to include on the page.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks. I was hoping there's a way the markdoc integration could tell about this information so we don't have to hardcode this, but if it's not easy to do so, I'm usually fine with things like this 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I mean it definitely could. We need a generic solution if we keep going down that path. Maybe using metadata or something like that... probably would rather hold off until the next time to tackle that.


const STRIP_QUERY_PARAMS_REGEX = /\?.*$/;
const ASTRO_PROPAGATED_ASSET_REGEX = /\?astroPropagatedAssets/;

/** recursively crawl the module graph to get all style files imported by parent id */
export async function* crawlGraph(
Expand All @@ -23,7 +23,6 @@ export async function* crawlGraph(
): AsyncGenerator<ModuleNode, void, unknown> {
const id = unwrapId(_id);
const importedModules = new Set<ModuleNode>();
if (new URL(id, 'file://').searchParams.has(PROPAGATED_ASSET_FLAG)) return;

const moduleEntriesForId = isRootFile
? // "getModulesByFile" pulls from a delayed module cache (fun implementation detail),
Expand All @@ -44,6 +43,7 @@ export async function* crawlGraph(
if (id === entry.id) {
scanned.add(id);
const entryIsStyle = isCSSRequest(id);

for (const importedModule of entry.importedModules) {
// some dynamically imported modules are *not* server rendered in time
// to only SSR modules that we can safely transform, we check against
Expand All @@ -60,15 +60,11 @@ export async function* crawlGraph(
if (entryIsStyle && !isCSSRequest(importedModulePathname)) {
continue;
}
const isFileTypeNeedingSSR = fileExtensionsToSSR.has(npath.extname(importedModulePathname));
if (
fileExtensionsToSSR.has(
npath.extname(
// Use `id` instead of `pathname` to preserve query params.
// Should not SSR a module with an unexpected query param,
// like "?astroPropagatedAssets"
importedModule.id
)
)
isFileTypeNeedingSSR &&
// Should not SSR a module with ?astroPropagatedAssets
!ASTRO_PROPAGATED_ASSET_REGEX.test(importedModule.id)
) {
const mod = loader.getModuleById(importedModule.id);
if (!mod?.ssrModule) {
Expand Down
4 changes: 3 additions & 1 deletion packages/astro/src/vite-plugin-head/index.ts
Expand Up @@ -9,7 +9,8 @@ import { getTopLevelPages, walkParentInfos } from '../core/build/graph.js';
import type { BuildInternals } from '../core/build/internal.js';
import { getAstroMetadata } from '../vite-plugin-astro/index.js';

const injectExp = /^\/\/\s*astro-head-inject/;
// Detect this in comments, both in .astro components and in js/ts files.
const injectExp = /(^\/\/|\/\/!)\s*astro-head-inject/;
matthewp marked this conversation as resolved.
Show resolved Hide resolved

export default function configHeadVitePlugin({
settings,
Expand All @@ -32,6 +33,7 @@ export default function configHeadVitePlugin({
seen.add(id);
const mod = server.moduleGraph.getModuleById(id);
const info = this.getModuleInfo(id);

if (info?.meta.astro) {
const astroMetadata = getAstroMetadata(info);
if (astroMetadata) {
Expand Down
5 changes: 5 additions & 0 deletions packages/astro/src/vite-plugin-markdown/content-entry-type.ts
Expand Up @@ -13,6 +13,8 @@ export const markdownContentEntryType: ContentEntryType = {
rawData: parsed.matter,
};
},
// We need to handle propagation for Markdown because they support layouts which will bring in styles.
handlePropagation: true,
};

/**
Expand All @@ -30,6 +32,9 @@ export const mdxContentEntryType: ContentEntryType = {
rawData: parsed.matter,
};
},
// MDX can import scripts and styles,
// so wrap all MDX files with script / style propagation checks
handlePropagation: true,
contentModuleTypes: `declare module 'astro:content' {
interface Render {
'.mdx': Promise<{
Expand Down
3 changes: 2 additions & 1 deletion packages/integrations/markdoc/components/Renderer.astro
@@ -1,4 +1,5 @@
---
//! astro-head-inject
import type { Config } from '@markdoc/markdoc';
import Markdoc from '@markdoc/markdoc';
import { ComponentNode, createTreeNode } from './TreeNode.js';
Expand All @@ -14,4 +15,4 @@ const ast = Markdoc.Ast.fromJSON(stringifiedAst);
const content = Markdoc.transform(ast, config);
---

<ComponentNode treeNode={createTreeNode(content)} />
<ComponentNode treeNode={await createTreeNode(content)} />