-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Improve MDX rendering performance #8533
Conversation
🦋 Changeset detectedLatest commit: a6c5ea4 The changes in this PR will be included in the next version bump. 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 |
import { toRemarkInitializeAstroData } from '@astrojs/markdown-remark/dist/internal.js'; | ||
import { compile as mdxCompile, type CompileOptions } from '@mdx-js/mdx'; | ||
import { setAstroData } from '@astrojs/markdown-remark/dist/internal.js'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using setAstroData
instead of toRemarkInitializeAstroData
remark plugin helps avoid needing to re-create the processor everytime we render.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not expose this from the package?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It still feels a bit internal-ish so I put it in internal.js
without thinking much. Need a bit of time to think about the ideal API, which I can also do though 😬
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think we should either expose it as is, or design a better API, but not use internal methods. I think it's ok if it's not the ideal API, we can change it later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aight I updated to export setVfileFrontmatter
instead from the main export, and removed setAstroData
in the latest force push rebased to main
.
const mdxPluginOpts: CompileOptions = { | ||
remarkPlugins: await getRemarkPlugins(mdxOptions), | ||
rehypePlugins: getRehypePlugins(mdxOptions), | ||
recmaPlugins: mdxOptions.recmaPlugins, | ||
remarkRehypeOptions: mdxOptions.remarkRehype, | ||
jsx: true, | ||
jsxImportSource: 'astro', | ||
// Note: disable `.md` (and other alternative extensions for markdown files like `.markdown`) support | ||
format: 'mdx', | ||
mdExtensions: [], | ||
}; | ||
|
||
let importMetaEnv: Record<string, any> = { | ||
SITE: config.site, | ||
}; | ||
let processor: ReturnType<typeof createMdxProcessor>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All these options are moved to createMdxProcessor
|
||
// Skip nonessential plugins during performance benchmark runs | ||
const isPerformanceBenchmark = Boolean(process.env.ASTRO_PERFORMANCE_BENCHMARK); | ||
|
||
export function recmaInjectImportMetaEnvPlugin({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This plugin is moved to recma-inject-import-meta-env.ts
as-is without changes
} | ||
|
||
export function rehypeApplyFrontmatterExport() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This plugin is moved to rehype-apply-frontmatter-export.ts
as-is without changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not very familiar with this code. I just left one comment.
if (astroData instanceof InvalidAstroDataError) | ||
throw new Error( | ||
// Copied from Astro core `errors-data` | ||
// TODO: find way to import error data from core |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now we have AstroError
:
import { AstroError } from "astro/errors";
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mostly copied the existing code over. We can follow-up a PR for this improvement 😬
Changes
NOTE: this merges into
markdown-perf
branch. When #8532 is merged, I'll rebase this then merge it.recmaInjectImportMetaEnv
as its own filerehypeApplyFrontmatterExport
as its own fileI tested it in the astro docs and there doesn't seem to be any improvements :( but this seems like a nice change overall otherwise.
Testing
Ran astro & mdx tests locally
Docs
n/a