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

Jump to bottom

Changing renderers to components #549

Closed
wooorm opened this issue Feb 21, 2021 · 2 comments · Fixed by #563
Closed

Changing renderers to components #549

wooorm opened this issue Feb 21, 2021 · 2 comments · Fixed by #563
Labels
💪 phase/solved Post is done 🧑 semver/major This is a change 💬 type/discussion This is a request for comments

Comments

@wooorm
Copy link
Member

wooorm commented Feb 21, 2021

Currently, react-markdown allows users to define element types (i.e., tag names such as 'br', a functional component, a class, etc) for markdown constructs (link, delete, break, etc).

This has problems:

  • Users don’t know about markdown names (thematicBreak) or markdown peculiarities (that there are fully formed links and link references, the paragraphs in spread lists don’t show up)
  • It makes it impossible to support rehype plugins, which work after markdown
  • Markdown treats raw HTML in markdown as a black box, making parsing it hard. Most of the open issues are about HTML parsing. Supporting rehype plugins comes with rehype-raw, which is proper solution

I think it would be better to switch to allowing users to define element types for HTML constructs instead, as that solves the three above issues. I call it components and suggest using that field, because I like that name better, and because it aligns with remark-react, rehype-react, and @mdx-js/mdx.

Having both renderers and components is not ideal:

  • Supporting two ways to do something is a bad user experience, so it would be temporal and we’d suggest moving to components
  • It’s not just this one option, also allowedTypes, disallowedTypes, and allowNode would also come with an mdast (remark) and hast (rehype) version. What if they conflict?
  • Figure out the original shape of an mdast node at the hast/rehype stage, which is passed as node to the renderer and would be needed to detect other fields passed to renderers, is complex. And what about a linkReference?

Given the complexity of supporting both and that it’d be temporary anyway, I don’t see it as viable to support both renderers and components for one (or two) versions, and would prefer to rip the band aid off, provided that there are clear docs on how to upgrade.
@wooorm wooorm added 🧑 semver/major This is a change 🙉 open/needs-info This needs some more info 💬 type/discussion This is a request for comments labels Feb 21, 2021
@wooorm wooorm mentioned this issue Feb 22, 2021
Merged
This was referenced Mar 2, 2021
Closed
Closed
@wooorm wooorm mentioned this issue Mar 21, 2021
Closed
@wooorm wooorm mentioned this issue Apr 9, 2021
Merged
wooorm added a commit that referenced this issue Apr 12, 2021
@wooorm
Replace renderers w/ components, remove HTML parser from core (#563)
77c680f 
* 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 wooorm added 💪 phase/solved Post is done and removed 🙉 open/needs-info This needs some more info labels Apr 12, 2021
@MuYunyun
Copy link

MuYunyun commented Jun 3, 2021

@wooorm hi, if I want to write React in .md file, is it correct to use @mdx-js/mdx in components props?

@wooorm
Copy link
Member Author

wooorm commented Jun 3, 2021 •
edited

If you mean JSX in markdown, yes, mdx-js/mdx and xdm do that. react-markdown does not do that.
You should probably not use both.

@wooorm wooorm mentioned this issue Jul 21, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
💪 phase/solved Post is done 🧑 semver/major This is a change 💬 type/discussion This is a request for comments
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Replace renderers w/ components, remove HTML parser from core remarkjs/react-markdown
2 participants
@wooorm @MuYunyun