-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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 optimize with sibling nodes #10887
Conversation
🦋 Changeset detectedLatest commit: 8c3a6fc 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 |
const objectPropertyNodes = child.data.estree.body[0]?.declarations?.[0]?.init?.properties; | ||
const objectPropertyNodes = | ||
child.data.estree?.body[0]?.declaration?.declarations?.[0]?.init?.properties; |
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.
The previous access was incorrect and never actually worked 😅 I should look into properly typing this in the future.
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.
Logic seems fine to me, but would it be possible to have a test? Perhaps a unit test using the plugin and checking the result.
Thanks, I'll add a test 👍 There's currently the |
I added a unit test. It now reveals that it generates some cursed JSX code that has attribute values with quotes extending multiple lines, but it still seems to parse properly and we don't have to manually escape the new lines, so leaving it like so for now. |
* fix(mdx): convert remark-images-to-component plugin to a rehype plugin (#10697) * Remove fs read for MDX transform (#10866) * Tag MDX component for faster checks when rendering (#10864) * Use unified plugin only for MDX transform (#10869) * Only traverse children and handle mdxJsxTextElement when optimizing (#10885) * Rename to `optimize.ignoreComponentNames` in MDX (#10884) * Allow remark/rehype plugins added after mdx to work (#10877) * Improve MDX optimize with sibling nodes (#10887) * Improve types in rehype-optimize-static.ts * Rename `ignoreComponentNames` to `ignoreElementNames` I think this better reflects what it's actually doing * Simplify plain MDX nodes in optimize option (#10934) * Format code * Minimize diff changes * Update .changeset/slimy-cobras-end.md Co-authored-by: Sarah Rainsberger <[email protected]> --------- Co-authored-by: Armand Philippot <[email protected]> Co-authored-by: Sarah Rainsberger <[email protected]>
Changes
There was a flaw with how
rehype-optimize-static.ts
worked before. Take this page for example https://docs.astro.build/en/concepts/why-astro/, which is a static page with no components.Before, the rehype plugin will only transform to something like this:
With this PR, it can go one step further and transform as:
Which is only a single line and a single string, which simplifies the AST nodes and runtime rendering.
Performance:
After multiple runs, the timings fluctuate a bit so it's hard to tell of the improvements. But averaging it, the total build time drop from 126s to 124s. I thought it would be more, but I'll take the small win 🥲
Testing
The
mdx-optimize
playground that should still pass. And I tested in the Astro docs repo.Docs
Added a changeset and updated the internal README explaining the new code.