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

feat: markdown config typechecking #2970

Merged
merged 19 commits into from
Apr 11, 2022
Merged
Show file tree
Hide file tree
Changes from 8 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/forty-plants-hope.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@astrojs/markdown-remark': patch
---

Improved type checking
5 changes: 5 additions & 0 deletions .changeset/small-months-speak.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'astro': patch
---

Improved markdown config type checking
1 change: 1 addition & 0 deletions packages/astro/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,7 @@
"@types/resolve": "^1.20.1",
"@types/rimraf": "^3.0.2",
"@types/send": "^0.17.1",
"@types/unist": "^2.0.6",
"@types/yargs-parser": "^21.0.0",
"astro-scripts": "workspace:*",
"chai": "^4.3.6",
Expand Down
14 changes: 12 additions & 2 deletions packages/astro/src/@types/astro.ts
Original file line number Diff line number Diff line change
Expand Up @@ -349,14 +349,24 @@ export interface AstroUserConfig {
*/
drafts?: boolean;

/**
* @docs
* @name markdown.mode
* @type {'md' | 'mdx'}
* @default `mdx`
* @description
* Control wheater to allow components inside markdown files ('mdx') or not ('md').
*/
mode?: 'md' | 'mdx';

Comment on lines +484 to +493
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this actually does anything right now, which is why we didn't include it!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does add rehypeJsx and rehypeExpressions. I could remove the MDX mode altogether if these plugins do nothing

Copy link
Member

Choose a reason for hiding this comment

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

Ah no you're right! I also saw that #2971 added escaping for expressions when mode === 'md'. So I think this is fine!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After doing some testing, the only difference I've found is that components are being rendered with mdx but completely ignored with md. Expressions works in both modes, tho.

/**
* @docs
* @name markdown.shikiConfig
* @type {ShikiConfig}
* @typeraw {Partial<ShikiConfig>}
* @description
* Shiki configuration options. See [the markdown configuration docs](https://docs.astro.build/en/guides/markdown-content/#shiki-configuration) for usage.
*/
shikiConfig?: ShikiConfig;
shikiConfig?: Partial<ShikiConfig>;

/**
* @docs
Expand Down
27 changes: 8 additions & 19 deletions packages/astro/src/core/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import type { AstroConfig, AstroUserConfig, CLIFlags } from '../@types/astro';
import type { Arguments as Flags } from 'yargs-parser';
import type * as Postcss from 'postcss';

import { astroMarkdownOptions } from '@astrojs/markdown-remark';
import * as colors from 'kleur/colors';
import path from 'path';
import { pathToFileURL, fileURLToPath } from 'url';
Expand All @@ -13,6 +14,12 @@ import postcssrc from 'postcss-load-config';
import { arraify, isObject } from './util.js';
import { appendForwardSlash, trimSlashes } from './path.js';

// These two imports make astroMarkdownOptions work
Copy link
Member

Choose a reason for hiding this comment

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

wahhh??? Would love an explanation why here, for future readers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Check out the new commit, I added a longer (not sure if better) explanation

// If you remove them, TypeScript throws an error similar to this:
// `The inferred type of 'AstroConfigSchema' cannot be named without a reference to '../../../markdown/remark/node_modules/@types/unist'. This is likely not portable. A type annotation is necessary`
import type {} from 'shiki';
import type {} from 'unist';

load.use([loadTypeScript]);

interface PostCSSConfigResult {
Expand Down Expand Up @@ -140,25 +147,7 @@ export const AstroConfigSchema = z.object({
})
.optional()
.default({}),
markdown: z
.object({
drafts: z.boolean().optional().default(false),
mode: z
.union([z.literal('md'), z.literal('mdx')])
.optional()
.default('md'),
syntaxHighlight: z
.union([z.literal('shiki'), z.literal('prism'), z.literal(false)])
.optional()
.default('shiki'),
// TODO: add better type checking
shikiConfig: z.any().optional().default({}),
remarkPlugins: z.array(z.any()).optional().default([]),
rehypePlugins: z.array(z.any()).optional().default([]),
})
.passthrough()
.optional()
.default({}),
markdown: astroMarkdownOptions.default({}),
Copy link
Member

Choose a reason for hiding this comment

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

Thanks again for your patience on this review @JuanM04!

My main comment is that we really benefit from having all of our config parsing logic here in one file and in one single parser object instantiation.

Reading this PR, I can see the value you've put into validation reuse, which is great. But, I'm also bouncing around to follow where the different parsers / validation logic lives, defined across multiple files, and even multiple packages, with multiple zod parser instances referencing each other.

If you'd like to improve the existing markdown: z.object(... validation logic defined here, I'm all for it. I could even see the value in a new Plugin validator that both remarkPlugins and rehypePlugins, which breaks up the single-object-validator that I love but for a valid, valuable reason.

But at the end of the day, Astro owns the "Astro Config" type and therefore its validation, so I'd like to keep it as simple as possible and only inside the Astro package, for our sake as maintainers.

@JuanM04 lmk what you think, and how you'd like to move forward!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're totally right. The idea behind the division was about separation of concerns, since @astrojs/markdown-remark is its own package, but I see how that can be a problem.

I'll integrate this configuration in a single place

vite: z.any().optional().default({}),
experimental: z
.object({
Expand Down
4 changes: 3 additions & 1 deletion packages/markdown/remark/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,13 @@
"shiki": "^0.10.1",
"unified": "^10.1.2",
"unist-util-map": "^3.0.1",
"unist-util-visit": "^4.1.0"
"unist-util-visit": "^4.1.0",
"zod": "^3.14.3"
},
"devDependencies": {
"@types/github-slugger": "^1.3.0",
"@types/prismjs": "^1.26.0",
"@types/unist": "^2.0.6",
"astro-scripts": "workspace:*"
}
}
9 changes: 3 additions & 6 deletions packages/markdown/remark/src/index.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import type { AstroMarkdownOptions, MarkdownRenderingOptions, ShikiConfig, Plugin } from './types';
import { MarkdownRenderingOptions, astroMarkdownOptions } from './types.js';

import createCollectHeaders from './rehype-collect-headers.js';
import scopedStyles from './remark-scoped-styles.js';
Expand All @@ -20,7 +20,7 @@ import rehypeStringify from 'rehype-stringify';
import rehypeRaw from 'rehype-raw';
import matter from 'gray-matter';

export { AstroMarkdownOptions, MarkdownRenderingOptions, ShikiConfig, Plugin };
export * from './types.js';

/** Internal utility for rendering a full markdown file and extracting Frontmatter data */
export async function renderMarkdownWithFrontmatter(
Expand All @@ -38,11 +38,8 @@ export const DEFAULT_REHYPE_PLUGINS = ['rehype-slug'];

/** Shared utility for rendering markdown */
export async function renderMarkdown(content: string, opts?: MarkdownRenderingOptions | null) {
let { remarkPlugins = [], rehypePlugins = [] } = opts ?? {};
let { mode, syntaxHighlight, shikiConfig, remarkPlugins, rehypePlugins } = astroMarkdownOptions.parse(opts ?? {});
const scopedClassName = opts?.$?.scopedClassName;
const mode = opts?.mode ?? 'mdx';
const syntaxHighlight = opts?.syntaxHighlight ?? 'shiki';
const shikiConfig = opts?.shikiConfig ?? {};
const isMDX = mode === 'mdx';
const { headers, rehypeCollectHeaders } = createCollectHeaders();

Expand Down
31 changes: 2 additions & 29 deletions packages/markdown/remark/src/remark-shiki.ts
Original file line number Diff line number Diff line change
@@ -1,34 +1,7 @@
import type * as shiki from 'shiki';
import { getHighlighter } from 'shiki';
import { visit } from 'unist-util-visit';

export interface ShikiConfig {
/**
* The languages loaded to Shiki.
* Supports all languages listed here: https://github.com/shikijs/shiki/blob/main/docs/languages.md#all-languages
* Instructions for loading a custom language: https://github.com/shikijs/shiki/blob/main/docs/languages.md#supporting-your-own-languages-with-shiki
*
* @default []
*/
langs?: shiki.ILanguageRegistration[];
/**
* The styling theme.
* Supports all themes listed here: https://github.com/shikijs/shiki/blob/main/docs/themes.md#all-themes
* Instructions for loading a custom theme: https://github.com/shikijs/shiki/blob/main/docs/themes.md#loading-theme
*
* @default "github-dark"
*/
theme?: shiki.IThemeRegistration;
/**
* Enable word wrapping.
* - true: enabled.
* - false: enabled.
* - null: All overflow styling removed. Code will overflow the element by default.
*
* @default false
*/
wrap?: boolean | null;
}
import type { ShikiConfig } from './types.js';

/**
* getHighlighter() is the most expensive step of Shiki. Instead of calling it on every page,
Expand All @@ -38,7 +11,7 @@ export interface ShikiConfig {
const highlighterCacheAsync = new Map<string, Promise<shiki.Highlighter>>();

const remarkShiki = async (
{ langs = [], theme = 'github-dark', wrap = false }: ShikiConfig,
{ langs, theme, wrap }: ShikiConfig,
scopedClassName?: string | null
) => {
const cacheID: string = typeof theme === 'string' ? theme : theme.name;
Expand Down
46 changes: 36 additions & 10 deletions packages/markdown/remark/src/types.ts
Original file line number Diff line number Diff line change
@@ -1,16 +1,42 @@
import type * as unified from 'unified';
import type { ShikiConfig } from './remark-shiki';
export { ShikiConfig };
import type { Node } from 'unist';
import type { ILanguageRegistration, IThemeRegistration, Theme } from 'shiki';
import { BUNDLED_THEMES } from 'shiki';
import { z } from 'zod';

export type Plugin = string | [string, any] | unified.Plugin | [unified.Plugin, any];
export { Node };
export type PluginFunction<PluginParameters extends any[] = any[], Input = Node, Output = Input> = unified.Plugin<PluginParameters, Input, Output>;

export interface AstroMarkdownOptions {
mode?: 'md' | 'mdx';
syntaxHighlight?: 'shiki' | 'prism' | false;
shikiConfig?: ShikiConfig;
remarkPlugins?: Plugin[];
rehypePlugins?: Plugin[];
}
const plugin = z.union([
z.string(),
z.tuple([z.string(), z.any()]),
z.custom<PluginFunction>((data) => typeof data === 'function'),
z.tuple([z.custom<PluginFunction>((data) => typeof data === 'function'), z.any()]),
]);

export type Plugin = z.infer<typeof plugin>;

const shikiConfig = z.object({
langs: z.custom<ILanguageRegistration>().array().default([]),
theme: z
.enum(BUNDLED_THEMES as [Theme, ...Theme[]])
.or(z.custom<IThemeRegistration>())
.default('github-dark'),
wrap: z.boolean().or(z.null()).default(false),
});

export type ShikiConfig = z.infer<typeof shikiConfig>;

export const astroMarkdownOptions = z.object({
mode: z.enum(['md', 'mdx']).default('mdx'),
drafts: z.boolean().default(false),
syntaxHighlight: z.union([z.literal('shiki'), z.literal('prism'), z.literal(false)]).default('shiki'),
shikiConfig: shikiConfig.default({}),
remarkPlugins: plugin.array().default([]),
rehypePlugins: plugin.array().default([]),
});

export type AstroMarkdownOptions = z.infer<typeof astroMarkdownOptions>;

export interface MarkdownRenderingOptions extends Partial<AstroMarkdownOptions> {
/** @internal */
Expand Down
6 changes: 6 additions & 0 deletions pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.