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

Idea: Leverage rehype to simplify transforms #427

Closed
ChristianMurphy opened this issue May 17, 2020 · 6 comments · Fixed by #563
Closed

Idea: Leverage rehype to simplify transforms #427

ChristianMurphy opened this issue May 17, 2020 · 6 comments · Fixed by #563
Labels
🗄 area/interface This affects the public interface 🙆 yes/confirmed This is confirmed and ready to be worked on

Comments

@ChristianMurphy
Copy link
Member

ChristianMurphy commented May 17, 2020

Hey @rexxars 👋
While looking through the project and saw an area to open up some new use cases and simplify the project in the process.

Currently the flow through react-markdown is

           +----------+        +----------------+        +--------------------------------------+
           |          |        |                |        |                                      |
-markdown->+  remark  +-mdast->+ remark plugins +-mdast->+ custom to html and to create element +-react element->
           |          |        |                |        |                                      |
           +----------+        +----------------+        +--------------------------------------+

where the translation to from markdown to html, the sanitizing, and the rendering to react elements all happen in the custom code section.

An alternative could be

           +----------+        +----------------+        +---------------+       +----------------+       +--------------+
           |          |        |                |        |               |       |                |       |              |
-markdown->+  remark  +-mdast->+ remark plugins +-mdast->+ remark-rehype +-hast->+ rehype plugins +-hast->+ rehype-react +-react elements->
           |          |        |                |        |               |       |                |       |              |
           +----------+        +----------------+        +---------------+       +----------------+       +--------------+

breaking out the translation from markdown to html into a step, and leveraging https://github.com/remarkjs/remark-rehype
allowing rehype plugins to further adapt the html (useful for sanitizing and extensions)
then translating the html to react elements https://github.com/rehypejs/rehype-react

The desired flow, markdown to react elements, with customization points is preserved.
The translation can be offloaded to rehype, reducing the amount and complexity of the code.
And it allows for customization, like remark-math which need to do transforms both on the markdown and html.

I built version of what this could look like here: https://github.com/ChristianMurphy/react-remark (with live demos https://christianmurphy.github.io/react-remark)
If you'd be interested I could open a pull request to pull the code over here.

Thoughts? 💭

@rexxars
Copy link
Collaborator

rexxars commented May 27, 2020

Hi @ChristianMurphy !

Thanks for taking the time to make such a thorough issue (and pull request!).

You seem to be quite familiar with the unified ecosystem and already have made this into a separate module (react-remark), and I was wondering what your thoughts are on remark-react, react-remark and now react-markdown?

The reason I created react-markdown was because I wanted a module with a simple API that people can easily understand. From a maintainer perspective, I welcome the change to push responsibility down to remark/rehype and it's plugins, but I'm not sure I think it creates the best end-user experience. For people who are unfamiliar with the unified ecosystem, syntax trees and the terms involved, it can be quite challenging to figure out what rehype, remark, hast, mdast and similar actually means and how to tie them all together.

The problem with a "simpler" API is of course that it (often) leads to a less flexible system (eg you can't use rehype plugins).

I want to explore a middle ground, where common options can be exposed and passed on to the respective unified component, while still allowing full flexibility if the user wants full control. How do you feel about that?

@ChristianMurphy
Copy link
Member Author

was wondering what your thoughts are on remark-react, react-remark and now react-markdown?

remark-react is a wrapper around remark-rehype and rehype-react, that could be used directly, but it seems allowing hooks into the rehype pipeline could allow for more extensibility.

react-remark I see as more an un-opinionated wrapper, just plain markdown with extension points for plugins.

react-markdown I see more as an opinionated wrapper around react-remark, which has some plugins already setup, can allows them to be configured through similar attributes/properties as exist today, but also offering extension points to add more plugins both for remark and rehype.

I'm not sure I think it creates the best end-user experience

Can you expand on this?
What would you see as missing from the shift?

For people who are unfamiliar with the unified ecosystem, syntax trees and the terms involved, it can be quite challenging to figure out what rehype, remark, hast, mdast and similar actually means and how to tie them all together.

I think most (if not all) the current options could be preserved, it would mostly shift responsibility of the rendering, and add a second plugin extension point, which would allow for more advanced plugins to work for people who want to build on the defaults.

I want to explore a middle ground, where common options can be exposed and passed on to the respective unified component, while still allowing full flexibility if the user wants full control. How do you feel about that?

I think we're on the same page

@ChristianMurphy ChristianMurphy added 🙆 yes/confirmed This is confirmed and ready to be worked on 🗄 area/interface This affects the public interface labels Oct 8, 2020
@ChristianMurphy
Copy link
Member Author

/cc @heri16 #470 (comment)

do you have a specific concern with rehype as an intermediate step?
This would use rehype under the hood, allowing for more advanced transforms, but wouldn't require users to directly interact with rehype.

@vasilii-kovalev
Copy link

Hello!

This comment about problems with remark-slug brought me here.
It leads to #384 and #428 (which leads to this one).
As far as I can see, all connected PRs in #384 and this one are resolved and closed, so I'm curious what's the status of these issues?
@ChristianMurphy

Thanks.

@wooorm
Copy link
Member

wooorm commented Dec 30, 2020

Soonish, I’m thinking maybe end of January or so for the next push. The reason is that too many breaking changes at once leads to a bad time for users. But this situation right now is not ideal either.

wooorm added a commit that referenced this issue Apr 12, 2021
* Replace `renderers` w/ `components`
* Replace `allowNode` w/ `allowElement`, which is now given a hast element (as
  the first parameter)
* Replace `allowedTypes` w/ `allowedElements`
* Replace `disallowedTypes` w/ `disallowedElements`
* Change signature of `linkTarget` and `transformLinkUri`, which are now given
  hast children (as the second parameter)
* Change signature of `transformImageUri`, which is now given the `alt` string
  as the second parameter (instead of the fourth)
* Replace `plugins` w/ `remarkPlugins` (backwards compatible change)
* Add `rehypePlugins`
* Change `includeNodeIndex` to `includeElementIndex`: it still sets an `index`,
  but that value now represents the number of preceding elements, it also sets a
  `siblingCount` (instead of `parentChildCount`) with the number of sibling
  elements in the parent
* The `columnAlignment` prop is no longer given to table elements: it’s
  available as `style` on `th` and `td` elements instead
* The `spread` prop is no longer given to list elements: it’s already handled

Remove buggy HTML parsers from core

* If you want HTML, add [`rehype-raw`](https://github.com/rehypejs/rehype-raw)
  to `rehypePlugins` and it’ll work without bugs!
* Remove `allowDangerousHtml` (previously called `escapeHtml`) option: pass
  `rehype-raw` in `rehypePlugins` to allow HTML instead
* Remove `with-html.js`, `plugins/html-parser.js` entries from library
* Remove naïve HTML parser too: either use `rehype-raw` to properly support
  HTML, or don’t allow it at all

Closes GH-549.
Closes GH-563.

The following issues are solved as rehype is now available:

Closes GH-522.
Closes GH-465.
Closes GH-427.
Closes GH-384.
Closes GH-356.

The following issues are solved as a proper HTML parser (`rehype-raw`) is now
available:

Closes GH-562.
Closes GH-460.
Closes GH-454.
Closes GH-452.
Closes GH-433.
Closes GH-386.
Closes GH-385.
Closes GH-345.
Closes GH-320.
Closes GH-302.
Closes GH-267.
Closes GH-259.

The following issues are solved as docs are improved:

Closes GH-251.
@wooorm
Copy link
Member

wooorm commented Apr 12, 2021

This should be solved by landing GH-563 today, which will soon be released in v6.0.0!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🗄 area/interface This affects the public interface 🙆 yes/confirmed This is confirmed and ready to be worked on
Development

Successfully merging a pull request may close this issue.

4 participants