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

Alternative to createMdxAstCompiler without mdxAstToMdxHast #1512

Closed
aleclarson opened this issue Apr 6, 2021 · 10 comments
Closed

Alternative to createMdxAstCompiler without mdxAstToMdxHast #1512

aleclarson opened this issue Apr 6, 2021 · 10 comments
Labels
💬 type/discussion This is a request for comments

Comments

@aleclarson
Copy link

I want an export like createMdxAstCompiler but I also want to preserve the MDX syntax tree, instead of getting a HTML syntax tree back.

This is for a Vite plugin that uses the local @mdx-js/mdx, so importing remark-parse, remark-mdx, etc is not viable.

@aleclarson aleclarson added 🙉 open/needs-info This needs some more info 🦋 type/enhancement This is great to have labels Apr 6, 2021
@aleclarson
Copy link
Author

Here's what I'm doing as a workaround: brillout/vite-plugin-mdx@818bdab

@ChristianMurphy
Copy link
Member

ChristianMurphy commented Apr 6, 2021

This seems like an XY problem.
Can you clarify what exactly you want to solve and where the gap in the existing packages is.

It also doesn't really make sense particularly around:

local @mdx-js/mdx, so importing remark-parse, remark-mdx, etc is not viable

@mdx-js/mdx is just a wrapper around remark-parse and remark-mdx

return unified()
.use(remarkParse)
.use(remarkMdx)

as you seem to be aware from your solution:

Here's what I'm doing as a workaround brillout/vite-plugin-mdx@818bdab

the imports looks a bit strange and unnecessarily complex, but otherwise it seems fine, I'm not sure where the root issue you are trying to solve is.

@ChristianMurphy ChristianMurphy added 🙉 open/needs-info This needs some more info and removed 🙉 open/needs-info This needs some more info 🦋 type/enhancement This is great to have labels Apr 6, 2021
@ChristopherBiscardi
Copy link
Member

@ChristianMurphy it's because without the remark AST it's harder to find the import nodes, or combine the ASTs from two different MDX files (prebuilt transclusion, etc). You can see where it's happening here in the linked plugin. (Also see #1423 for a similar, but not exactly the same use case).

I also have a use case for this in Sector, the block editor I'm building on top of MDX and the workaround I use is the same as the one posted here (copy/paste the createMdxAstCompiler bits without the hast plugin) and I've talked to @johno about including this behavior as if Sector needs it then others will as well to build more sophisticated tools on top of MDX.

@ChristianMurphy In the future, if you don't understand the use case please avoid linking to the xy problem question. You will get better responses from users if you don't link to 10 year old stackexchange questions.

@ChristianMurphy
Copy link
Member

ChristianMurphy commented Apr 6, 2021

I'm aware why the remark+MDX AST exists 🙂 .
I don't understand why remark + remark-mdx cannot be used.
the unified ecosystem is modular for a reason, pieces can be fit together, and don't strictly need core to export every possible combination.

In the future, if you don't understand the use case please avoid linking to the xy problem question. You will get better responses from users if you don't link to 10 year old stackexchange questions.

I welcome a better link for explaining the XY Problem.
The unified community has a diverse global audience, including many members who speak English as a second or third language, when jargon is used it should be linked to an appropriate definition.
Making the conversation inclusive and welcome is a core tenant of the unified community, and something I'd appreciate if you put some effort into as well, rather than criticizing.

@aleclarson
Copy link
Author

the imports looks a bit strange and unnecessarily complex

We have to import remark-parse, remark-mdx, etc from the root of @mdx-js/mdx installation, or else PNPM and local clones of vite-plugin-mdx won't respect the developer's desired @mdx-js/mdx version. Instead, it would be using the version from devDependencies of vite-plugin-mdx.

the unified ecosystem is modular for a reason, pieces can be fit together, and don't strictly need core to export every possible combination.

"every possible combination" is hyperbole. The only case I want to be supported out-of-the-box is "everything MDX does by default, excluding HTML-specific stuff".

@wooorm
Copy link
Member

wooorm commented Apr 6, 2021

I second that these questions raised here seem like XY problems: the original request for exposing such a function might be fine, but the solutions for vite-plugin-mdx might be done in another way, given that:

a) the custom import syntax is interesting, but novel to this particular plugin, and in my opinion unclear (import without assignment typically indicates something imported for side effects) whereas the in my opinion clearer:

import SomeContent from './example.mdx'

<SomeContent />

…works in other places already.

b) vite supports rollup plugins to compile mdx -> js, which when used would handle “recursion” (as in, one mdx file loading another)

@aleclarson what is the reason for this new syntax? Have you looked at using a rollup plugin? I was also looking at esbuild, but unfortunately vite doesn’t allow esbuild plugins. Might be something to ask vite folks about.

@aleclarson
Copy link
Author

import without assignment typically indicates something imported for side effects

The side effect is "pre-built transclusion", as @ChristopherBiscardi so eloquently put it.

what is the reason for this new syntax?

Convenience and intuitiveness. I originally tried this syntax hoping it would work.

Have you looked at using a rollup plugin?

Vite plugins are Rollup plugins, with added capabilities (like dev server hooks).

I was also looking at esbuild, but unfortunately vite doesn’t allow esbuild plugins. Might be something to ask vite folks about.

Not interested. I have a working solution already! :)

@aleclarson
Copy link
Author

I've hit another use case for having this feature:
In my Remark plugin, I want to parse an MDX string and insert that into the Remark AST, instead of building the AST by hand.

@ChristianMurphy
Copy link
Member

ChristianMurphy commented Apr 10, 2021

@aleclarson this comes back to the XY problem.
What I am hearing from you and @ChristopherBiscardi is that it is not clear how to access and work with the MDAST+MDX AST.
That is valid and valuable feedback, thank you both!

You've both suggested a solution, adding a new API to access the Markdown+MDX AST, inside @mdx-js/mdx.
Which is also appreciated, thank you again!

What is missing, is that this is not the only solution.
There are others, including:

  • Publishing a new utility package in the @mdx-js name space
  • Having community members publish a package with the utility method outside the @mdx-js namespace
  • Improving the documentation and guides with more information on the Markdown+MDX AST and how to work with it

Which leads to which approach should be taken?

Adding a new method to @mdx-js/mdx would offer a way to get the AST.
But would also increase the API surface, adding more to document and maintain, in addition, adding a second method similar to createMdxAstCompiler, which could confuse adopters, with care needed to understand which of several closely related APIs actually meets their needs.

An alternative, improving the documentation and guides to add more information on Markdown+MDX AST and how to work with it with the existing API, seems to address the core concern of making it more approachable and easier for new adopters to consider leveraging it, without increasing API surface or adding a potentially confusing method. This is the approach I would currently favor.

That said, the approaches are not mutually exclusive, a method could be added and the documentation improved.

"every possible combination" is hyperbole. The only case I want to be supported out-of-the-box is "everything MDX does by default, excluding HTML-specific stuff".

Not entirely.
MDX provides a method for accessing the full AST transform pipeline.

We could add one to access the Markdown+MDX AST specifically.
We could also add one to take the Markdown+MDX AST and transform it to HAST+MDX.
We could add ones which don't remove spaces or don't squeeze paragraphs.
All of the above would be valuable to someone, and for good reason too.
But how many belong as utilities maintained in @mdx-js/mdx?
Why or why not?

That seems like a broader discussion, which should include @mdx-js/core and @mdx-js/maintainers.

Personally I see these as better suited for documentation, than an ever increasing core API.
But am open and interested in reasoning that sees otherwise.

Thoughts? 💭

@ChristianMurphy ChristianMurphy added 💬 type/discussion This is a request for comments and removed 🙉 open/needs-info This needs some more info labels Apr 10, 2021
@wooorm
Copy link
Member

wooorm commented Oct 20, 2021

This particular issue subject can be solved by doing:

import {unified} from 'unified'
import remarkParse from 'remark-parse'
import remarkMdx from 'remark-mdx' // install with `@next` for now.

const processor = unified().use(remarkParse).use(remarkMdx)

This can be used like so: processor.parse('# Hi {1 + 1}') to get the AST from outside of MDX / a plugin.

For the vite-plugin-mdx-specific functionality of supporting:

# Hi

import "./partial.mdx"

^-- where the content of partial is injected into the document,

This can be achieved with a plugin using our new MDX 2 ASTs (see https://v2.mdxjs.com), either on the markdown or HTML AST.
Here’s some speudo-code that should be close to how it could work:

export default function remarkMdxTurnImportsWithoutSpecifiersIntoPartials() {
  return (tree) {
    const count = 0

    visit(tree, 'mdxjsEsm', (node, index, parent) => {
      const elements = []

      node.data.estree.body.forEach((importExport) => {
        // An import declaration w/o specifiers: `import "x"`
        // This should maybe check if it looks like an `.mdx?` import though?
        if (importExport.type === 'ImportDeclaration' && importExport.specifiers.length === 0) {
          const name = 'MDXPartial' + (count++)
          // Add a default specifier: `import MDXPartial1 from "x"`
          importExport.specifiers.push({type: 'ImportDefaultSpecifier', local: {type: 'Identifier', name}})
          // Add an element: `<MDXPartial1 />`
          elements.push({type: 'mdxJsxFlowElement', name, attributes: [], children: []})
        }
      })

      // Add the elements after the ESM:
      parent.children.splice(index + 1, 0, ...elements)
    })
  }
}

@wooorm wooorm closed this as completed Oct 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💬 type/discussion This is a request for comments
Development

No branches or pull requests

4 participants