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

Add mdx serialization support #153

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Add mdx serialization support #153

wants to merge 1 commit into from

Conversation

bnchi
Copy link
Contributor

@bnchi bnchi commented Oct 20, 2024

  • Add support for mdx_jsx and move over the tests

This PR is dependent on #152

@bnchi
Copy link
Contributor Author

bnchi commented Oct 20, 2024

@wooorm

This is roughly 60% finished, but I want to take your opinion on the way of how you're doing tracking and some of the (monkey patches) that are in the js version for mdx-js.

In mdx-js there's a copy of the containerFlow function here https://github.com/syntax-tree/mdast-util-mdx-jsx/blob/main/lib/index.js#L709

It's a modified copy which seems to have more logic in it for indentation, but it's only called from within the mdx-jsx module, my question here is does that mean mdx-jsx doesn't care about any of the previous tracking that have happened while we were serializing the tree ? If it's not the case do you think we should add support for tracking in mdast-util-to-markdown?

If we did so the Rust version might be out of sync with the JS one, but we can probably also make the same PR in the JS version, and I honestly don't know which parts of the tests are going to change (if any).

There are some other overridden options when we have a tree with jsx nodes here https://github.com/syntax-tree/mdast-util-mdx-jsx/blob/main/lib/index.js#L537

Does that mean in the Rust version at run time we need to know if we have a jsx node before we start serializing(this is obviously bad and costly), or should we let the user configure this option by themselves and error or panic when we find options passed with the wrong configuration ?

We can be explicit about the second option in the docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant