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

Exposes the independent steps into individual functions #27

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

arcanis
Copy link

@arcanis arcanis commented Apr 20, 2023

I'd like to access the original AST in order to extract the frontmatter data plus a couple of other information (like the headers, to generate a page navigation), but it wasn't possible before without parsing the file twice - possibly with different parsing options if I'm not careful. Additionally, I wanted to perform a couple of hast modifications before turning the tree into JS (in order to add heading anchors), but it wasn't exposed at all (neither on mdxjs-rs nor markdown-rs).

This diff solves that by splitting the various transforms into separate functions that the library consumer may call on their own. The crate still exports the "high-level" compile method, but by calling the steps manually one has much better control over the various trees.

It supercedes #8, since it makes it trivial to apply mutations to the trees before calling the next step (and I think it's better this way, because it becomes possible to perform async mutations, which wasn't possible with the callback approach in #8).

@arcanis arcanis changed the title Expose the original ast node Exposes the independent steps into individual functions Apr 22, 2023
@arcanis
Copy link
Author

arcanis commented Apr 22, 2023

I updated the PR to use a different approach which I found more flexible in my fork.

@wooorm
Copy link
Owner

wooorm commented Apr 24, 2023

We could go this way, temporarily, before we support plugins.
But we could also support plugins? And we can make them async?

I’m not opposed to utilities. That you as a somewhat advanced developer can mix and match together.
But plugins solve a need for reusable utilities, for non-advanced or non-rust developers.

@arcanis
Copy link
Author

arcanis commented Apr 24, 2023

And we can make them async?

I suppose the problem then becomes the opposite: what about people who don't want the compile function to be async 😄

That said, I don't mind if there's also a plugin API - I just find it convenient to have the primitives available. More power to advanced developers, as you said!

@wooorm
Copy link
Owner

wooorm commented Apr 27, 2023

In the JS world, plugins can return promises or not, and based on that, if a promise is returned but process is used instead of `processSync, an error is thrown.

I’m not sure how this would work exactly in Rust, but we could also have both SyncPlugin and Plugin, with processSync and process functions?

At least in the JS world, with promises, await, and top-level await, it’s much easier to use async stuff, and there aren‘t many reasons anymore to use sync stuff.
So perhaps we could also just go with an async process?

Copy link
Owner

@wooorm wooorm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At some point I’d want to externalize the steps into different crates.
It would also make sense to match the names that are used in the JS ecosystem.

So here are some proposals for different names

///
/// ## Errors
///
/// This project errors for many different reasons, such as syntax errors in
/// the MDX format or misconfiguration.
pub fn compile(value: &str, options: &Options) -> Result<String, String> {
pub fn parse_to_mdast(value: &str, options: &Options) -> Result<mdast::Node, String> {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’d prefer reusing the names that are currently used in the JS world, albeit that this one is specific to MDX instead of markdown, which doesn’t exist there.

How about mdast_util_from_mdx, inspired by mdast-util-from-markdown

/// This project errors for many different reasons, such as syntax errors in
/// the MDX format or misconfiguration.
pub fn mdast_to_hast(mdast: &mdast::Node) -> hast::Node {
mdast_util_to_hast(mdast)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be exposed as the full name: mdast_util_to_hast.

///
/// This project errors for many different reasons, such as syntax errors in
/// the MDX format or misconfiguration.
pub fn hast_to_program(
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hast_util_to_swc?

/// the MDX format or misconfiguration.
pub fn hast_to_program(
hast: &hast::Node,
original_source: &str,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should match value from earlier

Ok(program)
}

pub fn program_to_string(program: &mut Program) -> String {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

swc_util_to_js?

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.

2 participants