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

Ability to specify a custom text renderer/component removed in v6.0.0 #618

Closed
fastfedora opened this issue Jul 21, 2021 · 21 comments
Closed
Labels
👯 no/duplicate Déjà vu 🙋 no/question This does not need any changes

Comments

@fastfedora
Copy link

Subject of the issue

Previous versions of react-markdown had the ability to provide a custom text renderer (for example, to filter the text or wrap all text in a Material UI Typography element). This functionality appears to have been removed in version 6.0.0, with no mention in the changelog or release notes.

The only way I figured it out was noting there was no mapping for the old text render in this conversion chart and then searching through the code to find that references to the old text renderer were removed.

At a minimum, it would be convenient if the removal of this functionality was documented somewhere (this issue may provide that). But it would also be great to have this functionality back.

Old behavior

In 5.x and prior, a custom renderer could be provided via the text renderer that would apply to all text nodes:

  <ReactMarkdown
    renderers={{
      text: ({ children }) => <Typography variant="body1">{children}</Typography>
    }}
    ...
  />

Expected behavior

A mapping would exist for the components to provide the same functionality that was available in previous versions, where all text nodes were rendered with the custom renderer:

  <ReactMarkdown
    components={{
      text: ({ children }) => <Typography variant="body1">{children}</Typography>
    }}
    ...
  />

Actual behavior

There is no way to override the rendering of text nodes anymore.

@fastfedora fastfedora added 🐛 type/bug This is a problem 🙉 open/needs-info This needs some more info labels Jul 21, 2021
@Murderlon
Copy link
Member

Hi, this has been discussed and answered in this discussion: remarkjs/remark#700. Let us know if anything is still unclear.

Closing as this is a duplicate question: #609


@ChristianMurphy @wooorm perhaps we can update the docs as this seems to be a recurring issue?

@Murderlon Murderlon added 👯 no/duplicate Déjà vu 🙋 no/question This does not need any changes and removed 🐛 type/bug This is a problem 🙉 open/needs-info This needs some more info labels Jul 21, 2021
@fastfedora
Copy link
Author

I just found that issue. Apologies for the duplicate issue. Can the CHANGELOG for 6.0.0 perhaps be updated to indicate this though? That would make things a lot easier, since it seems several people are having the same challenge in upgrading.

@fastfedora
Copy link
Author

Also, discussion 700 proposes that the p component/renderer might be able to be used as a replacement. Is there anyone familiar with the codebase that can confirm that every text note is wrapped in a p? (I can try doing it, but I'm not familiar with the codebase).

@wooorm
Copy link
Member

wooorm commented Jul 21, 2021

No, not every text is in a p. And ps contain other things (such as emphasis).
You can inspect the syntax tree on AST Explorer (select markdown)

@wooorm
Copy link
Member

wooorm commented Jul 21, 2021

But why don’t you pass p: Typography? 🤔

@fastfedora
Copy link
Author

Okay, then is there a way to re-create the old functionality of providing a custom renderer for text nodes?

The solution in remarkjs/remark#700 doesn't seem to apply in my case, since the problem being solved there was a syntax problem, so using a plugin to modify the syntax tree doesn't seem appropriate here. I'm interested in modifying the rendering of the existing syntax tree, not modifying the tree.

Though if I need to modify the syntax tree to get the old behavior, I can do that as a hack. I just looked through all the remark and rehype plugins, though, and didn't see anything that allowed you to remap text nodes.

@fastfedora
Copy link
Author

I am essentially passing p: Typography. But previously this handled all text nodes, now it just handles p nodes. I guess my other alternative is to use the AST explorer you linked to to figure out what the syntax tree looks like for each Markdown type, and then override all the types that have text.

I did some testing, and, for instance, in versions prior to 6.0, the text renderer would be called for the text within a heading. Now, each h<1-6> needs to be explicitly overridden with another Typography element.

@wooorm
Copy link
Member

wooorm commented Jul 21, 2021

Why not use Material UI heading components that will also include roboto? 🤷‍♂️

@fastfedora
Copy link
Author

fastfedora commented Jul 21, 2021

I feel like you're missing the point. We can go through each of the changes—which I'm already doing—and fix each one. The point though is that before version 6.0 there was the ability to override the rendering of every text node with a single override. In 6.0, this functionality was removed and there is no easy way to replace the functionality.

Yes, there are solutions to each of the problems. I am currently going through and fixing them. But it requires knowing the implementation details of remark, which may change, so any potential solution is fragile.

For instance, overriding p works today because li appears to always wrap its contents in a p. But this is not required by HTML and it's reasonable a future version of this library decides to have a lighter DOM and remove unnecessary p elements. Then suddenly, a bunch more components need to be overridden.

For now, though, just having a list of which elements have their text node children wrapped in a p and which do not would be useful. I'm using the AST Explorer you linked to to figure this out by trial & error.

It would be helpful in the future to add the old functionality back into this library.

@wooorm
Copy link
Member

wooorm commented Jul 21, 2021

Yes, there are solutions to each of the problems. I am currently going through and fixing them. But it requires knowing the implementation details of remark, which may change, so any potential solution is fragile.

It requires knowing how markdown works. CommonMark prescribes how markdown maps to HTML. How CommonMark maps markdown to HTML is highly unlikely to ever change.

For instance, overriding p works today because li appears to always wrap its contents in a p. But this is not required by HTML and it's reasonable a future version of this library decides to have a lighter DOM and remove unnecessary p elements. Then suddenly, a bunch more components need to be overridden.

Some lis do not contain a p, but rather contain text directly: https://spec.commonmark.org/0.30/#loose.
This again is markdown/CommonMark and is highly unlikely to ever change.

It would be helpful in the future to add the old functionality back into this library.

Not going to happen. This change happened exactly to move away from custom APIs to what is standardized.

@ChristianMurphy
Copy link
Member

ChristianMurphy commented Jul 21, 2021

Can the CHANGELOG for 6.0.0 perhaps be updated to indicate this though?

in the changelog https://github.com/remarkjs/react-markdown/blob/main/changelog.md#change-renderers-to-components under "Show conversion table" this is noted.
Open to adding a link to the GitHub discussion in there as well.

But it requires knowing the implementation details of remark, which may change, so any potential solution is fragile.

How so? remark's API has been stable and relatively unchanged for at least 4-5 years.

reasonable a future version of this library decides to have a lighter DOM and remove unnecessary p elements

This isn't remark related, remark handles markdown, what you are describing are HTML/DOM changes.
Solving this at the remark level would make it robust to exactly this type of change.

It would be helpful in the future to add the old functionality back into this library.

I agree that more documentation and discussion around how to migrate away from text could be good.
Going back to the old API would be problematic, it had a number breaks where standard markdown wouldn't work and plain HTML would case crash (see linked issues at #427 for a few examples)

@fastfedora
Copy link
Author

fastfedora commented Jul 21, 2021

It may be the remark API has been stable, but, as a case in point, it does not match the spec linked to above for tight lists. The spec says that items in tight lists should be rendered without wrapping them in a paragraph, while you can go to the AST Explorer and see that remark v13.0.0 is creating an AST with paragraphs.

If remark is every brought in line with the spec at an implementation level, not an API level, then it will break anyone relying on the old implementation.

Note that I am not asking to go back to the entire old API. I am asking for a single piece of the old functionality, that of being able to override the rendering for text nodes, be restored, or an alternate solution provided.

HTML has the concept of text nodes just like markdown, so I don't see what the issue is. All that is required is the ability to override the rendered/component for text nodes, in addition to the other HTML node types that can be overridden by components.

@ChristianMurphy
Copy link
Member

ChristianMurphy commented Jul 21, 2021

It may be the remark API has been stable, but, as a case in point, it does not match the spec linked to above for tight lists

Again remark and mdast are not HTML, mdast paragraph is not the same as hast/HTML <p>.
remark does follow the CommonMark specification, and you can see an example of this at.
https://codesandbox.io/s/remark-rehype-debug-forked-iofgp

and the AST https://astexplorer.net/#/gist/38518decbafcee98ee1e8256fa383430/89ee2bdd76bb531f169aad7df6bf8382dd7f7d61 matches the CommonMark reference https://spec.commonmark.org/dingus/?text=*%20list%0A*%20item

@fastfedora
Copy link
Author

Okay, I see the difference now. I was reading the spec for converting Markdown -> HTML, and then was looking at the AST and interpreting it as the render tree, since it used the same render types as pre-6.0.0. But I see that the AST is then further transformed before being mapped into HTML.

Can you explain why a custom renderer cannot be provided for the text node then, when you can provide it for all the other nodes (currently expressed as HTML nodes, e.g. li, p, etc)?

(And I recognize the "text" node may be an AST text node or an HTML text node—unless you're saying there is some intermediate form that doesn't have text nodes, but has all the other nodes).

@wooorm
Copy link
Member

wooorm commented Jul 21, 2021

text, and other renderers, used to refer to markdown nodes. This required users to know about the AST. Now, with components, HTML element tag names need to be known. See here for the motivation: #549.

While technically text could be supported — everything is possible in remark / rehype / unified land — it doesn’t make a lot of sense: it’s not an element with a tag name. It doesn’t do what you expect (HTML text nodes also exist in code elements and as whitespace between elements).
I understand the question, but your proposed solution (adding text) is problematic. There are better solutions.

@fastfedora
Copy link
Author

Thanks. I read through the motivation before I upgraded to start doing my testing. It makes sense to me.

Where I disagree is that adding a solution to transform text doesn't make a lot of sense. It may not map into the rest of the ecosystem, but, unless I'm missing something, the component renderers are applied after all the other plugins have operated. They are applied at the end to render actual React elements. So isn't the entire idea of component renderers specific to the react-markdown library and not part of the broader ecosystem anyway?

While text is not an HTML tag name, text does exist as nodes in the DOM, and anyone who regularly inspects HTML DOMs would know what to expect. From a concept perspective, all that is required is to shift from the idea of rendering "tags" to rendering "nodes" (which, since node is passed in as a parameter to the component renderers, already seems to be the case in part of the API).

That all said, I'm not tied to the specific solution of passing a text renderer into components. Another alternative would be to provide a transformText function that can return a React element. Anything that provides a hook for the cross-cutting concern.

But you said there were better solutions. If there are others, please share. So far I've heard nothing but that I should learn exactly how the AST is parsed and rendered in CommonMark / remark / react-markdown, then override every single element that might render a text node just so the rendering of text nodes can be customized.

While doable, that's clearly a hack, when this used to be able to be done with a single override where the intent of the override was clear. It also goes against the spirit of 6.0.0 to make usage of this library simpler.

@fastfedora
Copy link
Author

Without a suitable alternative solution, I created my own. I added a transformText prop that serves as a central place to modify the rendering of all text rendered by react-markdown.

I submitted it via PR #619, in case it will be useful for others. I'm happy to make changes if you have suggestions. Or, if this solution doesn't work for you, feel free to close the PR. I switched my codebase to using this fork, which solved my remaining upgrade problems to v6.0.0.

@ChristianMurphy
Copy link
Member

An alternative would be to create a rehype plugin https://unifiedjs.com/learn/guide/create-a-plugin
which walks the HAST content https://unifiedjs.com/learn/recipe/tree-traversal/ and wraps Text nodes in a custom element.
which can then be handled with the components option https://github.com/remarkjs/react-markdown#architecture

I'm open to a transformText option. Thanks for opening a PR for review!

A consideration I've been thinking through, and it's not specific just to transformText, but also applies to other transform options like transformLinkUri, transformImageUri, linkTarget, skipHtml, etc.
Is how to give adopters a good mental model of when they happen and how they fit together.
One way would be to add documentation to https://github.com/remarkjs/react-markdown#architecture showing where they fit in the architecture, but some of them overlap a bit, and it could get a bit messy to document and reason about. 🤔

@fastfedora
Copy link
Author

My mental model for those is that the HAST has three types of transformation:

  • Attributes: linkUri, imageUri, linkTarget, though could be any metadata for an element extracted from Markdown or added in the processing pipeline that gets passed as a prop to the rendered component
  • Content: text within an element
  • Elements: nodes that get rendered into React elements via components (for Markdown elements) or passed-through as HTML elements (for HTML not skipped via skipHtml)

The transformations for attributes and content get applied first, then those get passed into the components as props to be rendered as React elements.

@ChristianMurphy
Copy link
Member

A concrete example of what a rehype/hast plugin that solves this problem looks like
https://codesandbox.io/s/react-markdown-wrap-text-79e70?file=/src/App.js

@ankurpiparsania
Copy link

I am essentially passing p: Typography. But previously this handled all text nodes, now it just handles p nodes. I guess my other alternative is to use the AST explorer you linked to to figure out what the syntax tree looks like for each Markdown type, and then override all the types that have text.

I did some testing, and, for instance, in versions prior to 6.0, the text renderer would be called for the text within a heading. Now, each h<1-6> needs to be explicitly overridden with another Typography element.

For anyone who is looking for similar use case, like accessing each h<1-6>, i have found it easier to use a Proxy that can be passed to components. This gives freedom to update multiple components at once if needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
👯 no/duplicate Déjà vu 🙋 no/question This does not need any changes
Development

Successfully merging a pull request may close this issue.

5 participants