feat: markdown config typechecking #2970

JuanM04 · 2022-04-02T19:50:42Z

Changes

Following the comment // TODO: add better type checking, I've added type checking for the markdown configuration

Testing

Docs

changeset-bot · 2022-04-02T19:50:44Z

🦋 Changeset detected

Latest commit: b01d281

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
@astrojs/markdown-remark	Patch
astro	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

packages/astro/src/@types/astro.ts

FredKSchott · 2022-04-02T19:53:23Z

packages/astro/src/core/config.ts

@@ -12,6 +13,9 @@ import loadTypeScript from '@proload/plugin-tsm';
 import postcssrc from 'postcss-load-config';
 import { arraify, isObject } from './util.js';
 import { appendForwardSlash, trimSlashes } from './path.js';
+// These two imports make astroMarkdownOptions work


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

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

packages/astro/src/core/config.ts

natemoo-re

I'd remove any public references to the mode option for now, since it doesn't do anything.

Other than that, looks great!

natemoo-re · 2022-04-03T19:04:14Z

packages/astro/src/@types/astro.ts

+		/**
+		 * @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';
+


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

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

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

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.

FredKSchott

Marking as red just to make sure we hold off on this until post release day. I have some comments that I'd like to make but just won't have the time until Monday/Tuesday to make them. Appreciate the patience!

FredKSchott · 2022-04-11T01:56:49Z

packages/astro/src/core/config.ts

-		.passthrough()
-		.optional()
-		.default({}),
+	markdown: astroMarkdownOptions.default({}),


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!

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

JuanM04 · 2022-04-11T13:41:20Z

@FredKSchott I've moved the validation inside config.ts. Also, I've removed all the internal references to the old markdownRender way of importing markdown, so it's cleaner and doesn't do unnecessary work

JuanM04 · 2022-04-11T17:41:09Z

Note: the error thrown by Deno it's not caused by this PR directly. If you have an Astro page with the <Markdown /> component and build it with Deno, it will throw "No Markdown parser found"

FredKSchott

Nice! This is looking great, thanks for making those changes. LGTM!

FredKSchott · 2022-04-11T18:55:06Z

Note: the error thrown by Deno it's not caused by this PR directly. If you have an Astro page with the <Markdown /> component and build it with Deno, it will throw "No Markdown parser found"

Can't merge with failing tests! What's the best way to resolve?

JuanM04 · 2022-04-11T19:05:22Z

Can't merge with failing tests! What's the best way to resolve?

I'm looking into it. Either a fix is found or the test is ignored

FredKSchott · 2022-04-11T19:09:04Z

As long as you can confirm that this isn't a regression caused by this PR

JuanM04 · 2022-04-11T20:23:04Z

Ok, this way it should work. The markdown plugin now is dynamically imported only when using <Markdown /> and Node.js. When using Deno, it throws an error because it isn't compatible

Sidenote: I've just found out that this error was encountered in another PR: #3036 (comment)

FredKSchott

LGTM!

* Added schemas to markdown plugin * Added new schemas to main package * Changesets * typeraw * Explaination about the weird type hack * Added markdown.mode to config * Added comment * Formatted * Moved validation to `astro` and added RemarkPlugin ad RehypePlugin * Removed the ability to have a custom markdown renderer internally * Fixed plugin type * Removed unused renderMarkdownWithFrontmatter * Added missing dependency * Dynamically import astro markdown * Cache import

JuanM04 added 2 commits April 2, 2022 16:40

Added schemas to markdown plugin

7d2575f

Added new schemas to main package

48a75cb

JuanM04 requested a review from natemoo-re April 2, 2022 19:50

github-actions bot added pkg: astro Related to the core `astro` package (scope) feat: markdown Related to Markdown (scope) labels Apr 2, 2022

JuanM04 added 2 commits April 2, 2022 16:51

Merge branch 'main' into feat/new-config-md

ab59fcf

Changesets

6e2a770

FredKSchott reviewed Apr 2, 2022

View reviewed changes

packages/astro/src/@types/astro.ts Outdated Show resolved Hide resolved

FredKSchott reviewed Apr 2, 2022

View reviewed changes

packages/astro/src/core/config.ts Outdated Show resolved Hide resolved

JuanM04 added 4 commits April 2, 2022 16:53

typeraw

915abcc

Explaination about the weird type hack

34515c4

Added markdown.mode to config

32e4659

Merge branch 'main' into feat/new-config-md

bacf5c0

natemoo-re requested changes Apr 3, 2022

View reviewed changes

natemoo-re approved these changes Apr 3, 2022

View reviewed changes

FredKSchott self-requested a review April 4, 2022 06:22

FredKSchott requested changes Apr 4, 2022

View reviewed changes

JuanM04 added 3 commits April 10, 2022 15:47

Merge branch 'main' into feat/new-config-md

7a0a853

Added comment

9ca8234

Formatted

98fd50d

FredKSchott requested changes Apr 11, 2022

View reviewed changes

JuanM04 added 4 commits April 11, 2022 09:31

Merge branch 'main' into feat/new-config-md

df1f3ee

Moved validation to astro and added RemarkPlugin ad RehypePlugin

f77ac34

Removed the ability to have a custom markdown renderer internally

210496f

Fixed plugin type

8159fe9

JuanM04 added 2 commits April 11, 2022 11:07

Removed unused renderMarkdownWithFrontmatter

25b51ff

Added missing dependency

440f604

FredKSchott approved these changes Apr 11, 2022

View reviewed changes

JuanM04 added 2 commits April 11, 2022 17:11

Dynamically import astro markdown

ebca862

Cache import

b01d281

FredKSchott approved these changes Apr 11, 2022

View reviewed changes

FredKSchott merged commit b835e28 into main Apr 11, 2022

FredKSchott deleted the feat/new-config-md branch April 11, 2022 23:01

github-actions bot mentioned this pull request Apr 11, 2022

[ci] release #3080

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: markdown config typechecking #2970

feat: markdown config typechecking #2970

JuanM04 commented Apr 2, 2022

changeset-bot bot commented Apr 2, 2022 •

edited

Loading

FredKSchott Apr 2, 2022

JuanM04 Apr 2, 2022

natemoo-re left a comment

natemoo-re Apr 3, 2022

JuanM04 Apr 3, 2022

natemoo-re Apr 3, 2022

JuanM04 Apr 3, 2022

FredKSchott left a comment

FredKSchott Apr 11, 2022

JuanM04 Apr 11, 2022

JuanM04 commented Apr 11, 2022

JuanM04 commented Apr 11, 2022 •

edited

Loading

FredKSchott left a comment

FredKSchott commented Apr 11, 2022

JuanM04 commented Apr 11, 2022

FredKSchott commented Apr 11, 2022

JuanM04 commented Apr 11, 2022 •

edited

Loading

FredKSchott left a comment

feat: markdown config typechecking #2970

feat: markdown config typechecking #2970

Conversation

JuanM04 commented Apr 2, 2022

Changes

Testing

Docs

changeset-bot bot commented Apr 2, 2022 • edited Loading

🦋 Changeset detected

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natemoo-re left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FredKSchott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JuanM04 commented Apr 11, 2022

JuanM04 commented Apr 11, 2022 • edited Loading

FredKSchott left a comment

Choose a reason for hiding this comment

FredKSchott commented Apr 11, 2022

JuanM04 commented Apr 11, 2022

FredKSchott commented Apr 11, 2022

JuanM04 commented Apr 11, 2022 • edited Loading

FredKSchott left a comment

Choose a reason for hiding this comment

changeset-bot bot commented Apr 2, 2022 •

edited

Loading

JuanM04 commented Apr 11, 2022 •

edited

Loading

JuanM04 commented Apr 11, 2022 •

edited

Loading