Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Changes from 5 commits
3a5c533
e037f6a
28b2b5f
0f6aa4a
033bf16
d3dfb72
dd37711
c67e5e1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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 anif
block before anotherif
block. Logically, we can enter bothif
, 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:
Can be turned into:
So
Bar
here is something we want to track because in theleave
hook later, we can add it toallPossibleElements
.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 understandThere 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.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.