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

[markdown] Harder, better, faster, stronger vite-plugin-markdown #4137

Merged
merged 32 commits into from
Aug 5, 2022

Conversation

bholmesdev
Copy link
Contributor

@bholmesdev bholmesdev commented Aug 3, 2022

Changes

  • Rework vite-plugin-markdown to 1 compilation step
    • No more Astro + esbuild compilation, no more 2 step imports, no more mysterious ?content and ?mdImport flags 🥳
    • Pass through renderMarkdown in all cases, including globs - unblocks remark / rehype plugins appending to frontmatter
    • Use Astro's JSX runtime to render markdown layouts
    • Avoid switch logic on legacy.astroFlavoredMarkdown
  • Fall back to old vite-plugin-markdown (now renamed vite-plugin-markdown-legacy) based on legacy flag
  • Enable JSX runtime in all cases, not just MDX!
    • Tweak JSX renderer logic to better handle Astro renderer (based on failing unit tests)
  • Add headings and frontmatter to layout props, for consistency with MDX ([MDX] Add headings and frontmatter to layout props #4134)

Testing

  • move astro-markdown suite to legacy-astro-flavored-markdown for clarity
  • port legacy tests to astro-markdown suite (formerly astro-markdown-md-mode): layouts, headings, import.meta.env, raw and compiled content

Docs

N/A

@changeset-bot
Copy link

changeset-bot bot commented Aug 3, 2022

🦋 Changeset detected

Latest commit: 95e4d8c

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

This PR includes changesets to release 13 packages
Name Type
astro Minor
@astrojs/markdown-remark Patch
@e2e/astro-component Patch
@e2e/error-react-spectrum Patch
@e2e/error-sass Patch
@e2e/errors Patch
@e2e/lit-component Patch
@e2e/preact-component Patch
@e2e/react-component Patch
@e2e/solid-component Patch
@e2e/svelte-component Patch
@e2e/e2e-tailwindcss Patch
@e2e/ts-resolution 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

@github-actions github-actions bot added pkg: astro Related to the core `astro` package (scope) feat: markdown Related to Markdown (scope) labels Aug 3, 2022
@bholmesdev bholmesdev force-pushed the feat/vite-plugin-markdown-rework branch from b57d625 to 5e24cb5 Compare August 3, 2022 22:25
@github-actions github-actions bot added the pkg: example Related to an example package (scope) label Aug 3, 2022
const title = content.title;
const headings = content.astro.headings;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

content.astro.headings now throws! You'll get a nice error recommending Astro.props.headings instead 👍

@@ -29,7 +29,7 @@ export function* getTopLevelPages(
): Generator<string, void, unknown> {
for (const info of walkParentInfos(id, ctx)) {
const importers = (info?.importers || []).concat(info?.dynamicImporters || []);
if (importers.length <= 2 && importers[0] === resolvedPagesVirtualModuleId) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This assumed a pages entry would never be imported, which breaks glob imports for MD files in /pages. The ?mdImport flag hid this bug before!

Copy link
Contributor

Choose a reason for hiding this comment

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

I fixed this a different way in #4156

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, glad to hear it! Should I revert and let your PR go first @matthewp ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update: Rebased with your fix 🙏

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, sorry I forgot to reply here.

// Why not the "transform" hook instead of "load" + readFile?
// A: Vite transforms all "import.meta.env" references to their values before
// passing to the transform hook. This lets us get the truly raw value
// to escape "import.meta.env" ourselves.
Copy link
Member

Choose a reason for hiding this comment

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

why do we want to escape import.meta.env ourselves instead of letting Vite do it?

Copy link
Contributor Author

@bholmesdev bholmesdev Aug 4, 2022

Choose a reason for hiding this comment

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

Because Vite does not escape them! It'll replace all import.meta.env usages with their values before giving you the code, since Vite assumes all modules are JS modules. A manual file read is the only way around it

Copy link
Member

Choose a reason for hiding this comment

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

Ohhhh right, we want them escaped. Makes sense! This is probably more accurate from a Rollup/Vite perspective as well, since I think load() was designed to load the file from disk and turn it into JS, while transform() is just for transforming the file without changing its file type.

Copy link
Member

@FredKSchott FredKSchott left a comment

Choose a reason for hiding this comment

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

LGTM overall, but left one comment on how to improve the output to reduce potentially-expensive JSON.stringify calls.

packages/astro/src/vite-plugin-markdown/index.ts Outdated Show resolved Hide resolved
Copy link
Member

@natemoo-re natemoo-re left a comment

Choose a reason for hiding this comment

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

LGTM assuming all other feedback gets addressed! Great work on this, Ben!

@bholmesdev bholmesdev force-pushed the feat/vite-plugin-markdown-rework branch from 498f6f4 to 37b805b Compare August 5, 2022 14:49
@bholmesdev bholmesdev merged commit 471c6f7 into main Aug 5, 2022
@bholmesdev bholmesdev deleted the feat/vite-plugin-markdown-rework branch August 5, 2022 19:43
@astrobot-houston astrobot-houston mentioned this pull request Aug 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: markdown Related to Markdown (scope) pkg: astro Related to the core `astro` package (scope) pkg: example Related to an example package (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants