-
-
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
Add MDX optimize option #7151
Add MDX optimize option #7151
Conversation
🦋 Changeset detectedLatest commit: c67e5e1 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 |
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.
Thanks @bluwy ! I left some suggestions for your consideration below because I think we can try to make the explanations less implementation-y and more explanatory to the reader when and why to use this config option! See what you think, and let me know where I've gotten things wrong or missed something! 😅
@@ -183,6 +183,42 @@ These are plugins that modify the output [estree](https://github.com/estree/estr | |||
|
|||
We suggest [using AST Explorer](https://astexplorer.net/) to play with estree outputs, and trying [`estree-util-visit`](https://unifiedjs.com/explore/package/estree-util-visit/) for searching across JavaScript nodes. | |||
|
|||
### `optimize` |
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.
@bluwy Can you also add somthing around line 87 linking to this section?
Also, I'm wondering about the order of these 4 subsections. Optimizing sounds like a pretty important thing, so I might be tempted to put it first if you think it's going to be a no-brainer that most people would enable this.
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.
Good catch! I'm thinking of putting this last as a special feature we can point people to turn on if they're having issues. Ideally in the next breaking release when we get good feedback, we can make it the default so the option won't stay here long.
Thanks for the review @sarah11918. I've updated the docs with hopefully friendlier explanations now 😄 |
// stack for this node, things will still work but we won't repeatedly | ||
// run the above for other nodes anymore. If this is confusing, you can | ||
// comment out the code below when reading. | ||
elementStack.length = 0; |
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.
nit: What confuses me is that we drain the elementStack
inside an if
block before another if
block. Logically, we can enter both if
, which means we can drain the stack, push nodes, and then do it again.
The comment fails to address that.
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 the idea is that we can and want to enter both if block, because if this node is non-optimizable, that doesn't mean we can't optimize the children into this node.
For example:
<Foo>
<Bar>
# this is markdown
</Bar>
</Foo>
Can be turned into:
<Foo>
<Bar set:html="<h1>this is markdown</h1>"></Bar>
</Foo>
So Bar
here is something we want to track because in the leave
hook later, we can add it to allPossibleElements
.
I don't think it's easy to explain this as a comment because this optimization only comes clear when you read it last after understanding the (unfortunately complex 😅) algorithm.
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 wouldn't mind having a README.md
next to this file that explains a bit about what we do :) it's not always easy to explain what's on our mind in a few comments! I completely understand
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 think that's a good idea. I'll note a followup PR to document this so the docs can be reviewed separately.
Great PR! 🚀 |
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.
Docs LGTM!
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.
Looks great, @bluwy! Thanks for revisiting this! Just left a couple of thoughts! 🙌
Co-authored-by: Sarah Rainsberger <[email protected]>
Thanks @sarah11918. The couple suggestions look great to me. I made some small whitespace adjustments before merging your suggestions. |
Changes
Add
optimize
option for@astrojs/mdx
to optimize the MDX output for faster builds and rendering.Most of the rehype code is copied from https://github.com/withastro/docs/blob/6b6f629e2241dce12dd4763124d00eeb223aa1f4/plugins/rehype-optimize-static.ts, but:
export const components
and from a config option)Testing
Added tests to
@astrojs/mdx
. Also manually tested with Astro docs.Docs
Added docs for the new option in the
@astrojs/mdx
README./cc @withastro/maintainers-docs for feedback!