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 recmaJsxPlugins hook point, and update docs accordingly #2584

Closed
wants to merge 3 commits into from

Conversation

egnor
Copy link
Contributor

@egnor egnor commented Jan 28, 2025

Initial checklist

  • I read the support docs
  • I read the contributing guide
  • I agree to follow the code of conduct
  • I searched issues and discussions and couldn’t find anything or linked relevant results below
  • I made sure the docs are up to date
  • I included tests (or that’s not needed)

Description of changes

Per discussion at https://github.com/orgs/mdx-js/discussions/2581 I think it might make sense to add another
point for recma plugins which want to process Javascript with JSX tags still in, before the JSX is rewritten.
My use case is to implement pseudotags like <$if> and <$for> but I could imagine a number of other use cases.
This is the point at which tag content is maximally consistent -- Markdown, HTML-in-markdown and
components-in-markdown have all been rewritten into standard JSX nodes at this point.

I also expanded the documentation a bit to clarify exactly where in the stack the user plugins are added,
since that would have been helpful for me to know.

Copy link

vercel bot commented Jan 28, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
mdx ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 31, 2025 4:21am

@github-actions github-actions bot added 👋 phase/new Post is being triaged automatically 🤞 phase/open Post is being triaged manually and removed 👋 phase/new Post is being triaged automatically labels Jan 28, 2025
@codecov-commenter
Copy link

codecov-commenter commented Jan 28, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (908ff45) to head (d98775c).
Report is 79 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main     #2584   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           23        21    -2     
  Lines         2693      2653   -40     
  Branches         2         2           
=========================================
- Hits          2693      2653   -40     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@wooorm
Copy link
Member

wooorm commented Jan 29, 2025

I could imagine a number of other use cases.

Such as?

// Should this come before rehypePlugins?

No, that would break things; any remaining raw nodes need to be removed at that point. But plugins can handle raw nodes, that’s what rehype-raw does

// Move recmaStringify after recmaPlugins?

No, that would not change things, that’s a compiler.


Generally: I am not so sure this is a good solution. I also am not sure there is a problem.
You only look at JSX, which is one thing in MDX, but there are different things.
Your use case is about JSX, hence you want to do something before one plugin and after another.
But there are other cases: someone would want to do things before or after other plugins. This is what your inline comments are about too. So you discovered the limitations of this solution.

As for the problem, #2581, I do not think you need to solve it like this.
Like I mentioned in my last comment there, I think a rehype plugin is fine.
You do not care about elements which are injected by plugins, you do not have to look at JSXElement in expressions, you can look at mdxJsxFlowElement and mdxJsxTextElement, which in rehype are equivalent already.

Do you need help with how to do things in rehype?

@egnor
Copy link
Contributor Author

egnor commented Jan 29, 2025

If I respun this to just include the documentation changes, would you consider that? They would have been helpful to me.

The processing/deletion of raw nodes is a subtlety I don't think I understand. I'd dropped that comment mainly because it contradicted the claim I added to the document that user supplied rehype plugins are run "just before" the HTML tree is converted to JS -- in reality there's that one last step. I guess we could view that step as part of "conversion to JS".

As far as use cases I can imagine wanting to do various things at the "JSX level", treating tags and text content in a uniform way regardless of whether they're at the MDX top-level or derived from markdown or included as explicit JSX or inside nested JS expressions. Support for pseudotag "macros" like my $if, various styling tweaks, text content rewriting, styling changes, content standards checking, etc. might all prefer to be done at this level. As discussed in the other thread, at the rehype level the same content comes in many "flavors" depending on how it was created originally.

I do see that as you say there could be reasons to inject plugins at various points in the pipeline, or even to split apart some of the things you do in one phase. After all any plugin architecture has its limits. But this seemed like an easy harmful [edit] harmless addition that makes sense to me and feels like it could be generally useful for other general purpose content transformers. Clearly you disagree.

@wooorm
Copy link
Member

wooorm commented Jan 30, 2025

If I respun this to just include the documentation changes, would you consider that? They would have been helpful to me.

Some of it can help. Some of it is too much focussed on JSX, which is your use case, but other people have other use cases.

treating tags and text content in a uniform way regardless of whether they're at the MDX top-level or derived from markdown or included as explicit JSX

So, most of this can be chosen already: in the remark/mdast/markdown space, there is a small semantic difference between text and flow. In the rehype/hast/html space, one typically can treat those two names exactly the same, but the name is still there if one wants to differentiate.

I have thought about turning flow and text into one node in the rehype/hast/html space. The upside is that it is a simpler AST. The downside is: it’s more work, what does it actually improve?

or inside nested JS expressions

So, here, I would think this is a feature instead of a bug. MDX too treats things differently, whether they are in braces or not. Whether the author is “in markdown” or “in javascript”. Whether * works or not. Whether # works or not. Whether <h1> can be passed. Whether whitespace is collapsed or not.

(Note also that JSX can be in exports too: export function Button() { return <button .../> }; plugins could also add other )

But this seemed like an easy harmful addition that makes sense to me and feels like it could be generally useful for other general purpose content transformers. Clearly you disagree.

Every addition to a pretty stable tool used by many people is in some ways harmful. Makes tools harder to learn. Especially if it isn’t correctly thought out. In the future, someone else will come along that wants to inject some plugin here or there or wherever. That would need different fields. Would this then be deprecated and maintained for years to come?

So, I need clearly thought out use cases, where there are no acceptable alternatives. I, in this case, for now, think that doing things in the rehype/hast/HTML space is sufficiently adequate: https://github.com/orgs/mdx-js/discussions/2581#discussioncomment-12005963.

There are similar ideas in the design space, or perhaps you know “design by committee”. Stuff like that.

@egnor
Copy link
Contributor Author

egnor commented Jan 31, 2025

I've removed functionality changes (the whole recmaJsxPlugins thing, kept only documentation changes, and updated the documentation to be less JSX specific. Take another look?

Copy link
Member

@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.

Hi again!

At the bottom of the readme, there is the section “Architecture”.
I think that is the section you are trying to recreate: https://mdxjs.com/packages/mdx/#architecture.

These docs you changed are for everyone. Most of those people are people that have plugins and want to use them. Plugin names are the important part there. Many users will, for example, want to use remark-toc. Their primary question is: where does that go? rehypePlugins?

I believe that adding much of these changes, especially things like “(with JSX exceptions if jsx: true is set)” will make it hard for those users that know less, and care less, for the internals. Most users.

Given that you had a problem, I am wondering: did you not see the architecture section before? Was it missing things?

A couple years went by and now recma is published, there are some plugins — so there are definitely improvements to be made to the docs around that

@egnor
Copy link
Contributor Author

egnor commented Feb 3, 2025

😭

yes, I did find that architecture section, and read it repeatedly when I was trying to figure things out. I found it useful but also fairly high level, like it says "oh, remark ecosystem goes here" as opposed to a more precise statement of the order things happen -- specifically there are steps which are not plugins -- I had to write various test plugins to dump trees (and read the core.js file and others) to extract the specific knowledge I was trying to distill for the next intrepid visitor

so I guess maybe I could propose some elaboration of the architecture section with a bit more detail in the timeline and some information about exactly what to expect in terms of the nodes that can be encounter3d at any given plugin phase

but I don't think it's worth it! we always argue 😢

@egnor egnor closed this Feb 3, 2025

This comment has been minimized.

wooorm added a commit that referenced this pull request Feb 4, 2025
@wooorm
Copy link
Member

wooorm commented Feb 4, 2025

the order things happen

Hmm. I see a short numbered list of 8 steps. Then these steps are again explained under that, with more explanation and reasoning. I do not really see that the order of things is vague.

I had to write various test plugins to dump trees (and read the core.js file and others) to extract the specific knowledge I was trying to distill for the next intrepid visitor

There’s a playground: https://mdxjs.com/playground/. It shows the ASTs at different stages. Perhaps that would have helped you.

I also do think reading code is fine. It’s great to read code! You wanted to do complex things. You disagreed with my advice on doing it in rehype. That’s all fine. But then there will be some gotchas. And looking at the code is needed.

we always argue

Well, that’s why job, to talk, I need to understand your thinking and problems; to figure out something that is good for everybody.


I reworded some of the docs regarding recma and architecture in 35ac59d. Thanks!

@wooorm wooorm added the 💪 phase/solved Post is done label Feb 4, 2025
@github-actions github-actions bot removed the 🤞 phase/open Post is being triaged manually label Feb 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💪 phase/solved Post is done
Development

Successfully merging this pull request may close these issues.

3 participants