Skip to content
This repository was archived by the owner on Feb 15, 2022. It is now read-only.

Implement rendering an html AST within a react component #285

Closed
ghost opened this issue Apr 10, 2021 · 13 comments
Closed

Implement rendering an html AST within a react component #285

ghost opened this issue Apr 10, 2021 · 13 comments
Assignees

Comments

@ghost
Copy link

ghost commented Apr 10, 2021

Most js markdown rendering frameworks (next-mdx-remote, rehype, ...) offer the option to:

  1. output an html string combined with dangerously set html
    Or
  2. output react elements

Investigate the drawbacks of using an html string and decide on whether any of those drawbacks are show stoppers that mandate using react elements.

Another way to think about this question is what format should we use to serialize the data between getStaticProps and the React component rendering the output. Should the data be a javascript string of html or should the data be a javascript object tree of React elements?

@ghost ghost self-assigned this Apr 10, 2021
@agarwal
Copy link
Member

agarwal commented Apr 13, 2021

Investigate the drawbacks of using an html string and decide on whether any of those drawbacks are show stoppers that mandate using react elements.

Your phrasing implies a preference for outputting html. Any particular reason?

@ghost
Copy link
Author

ghost commented Apr 13, 2021

Any particular reason?

There are more options if we use html strings directly. For example, omd could be used without awaiting the implementation of additional features. We could also use simpler libraries inside of unified-js than rehype-react or mdxjs.

@ghost
Copy link
Author

ghost commented Apr 21, 2021

I have been aware of the limitation where links from markdown content to other pages within the site will cause a full page reload if html is used instead of react.elements (and next/link). I haven't been able to think of any other drawbacks to using html directly. Links between markdown pages will not occur often and a full page refresh is barely noticable. Most tutorial markdown files are long blocks of text without links.

I recommend we forgo rehype-react, use the much simpler rehype-stringify output. This will also open up the possibility of replacing remark completely with omd or another js markdown library, at some point.

@ghost
Copy link
Author

ghost commented Apr 21, 2021

I decide to switch to outputting html instead of React elements. After #296 is merged, there will not be any more references to mdxjs. I didn't formally do a spike, but I'll close this issue as done. I will reopen this issue if we find that this decision was incorrect.

@ghost ghost closed this as completed Apr 21, 2021
@agarwal
Copy link
Member

agarwal commented Apr 22, 2021

the limitation where links from markdown content to other pages within the site will cause a full page reload if html is used instead of react.elements (and next/link). I haven't been able to think of any other drawbacks to using html directly. Links between markdown pages will not occur often and a full page refresh is barely noticable

Not sure I agree with this. Full page reloads are quite wrong. I appreciate they may be fast enough in our use case, but this makes me feel we're doing it wrong.

@ghost ghost reopened this Apr 22, 2021
@ghost
Copy link
Author

ghost commented Apr 22, 2021

this makes me feel we're doing it wrong.

I think the most technically robust implementation would use rehype-react to produce React elements. This would most likely rule out switching to omd within the next few years.

@agarwal agarwal unassigned ghost Apr 22, 2021
@ghost
Copy link
Author

ghost commented Apr 23, 2021

The following information is not important, but it may help inform our decision more. The graphs present the dependencies of each option, colored by maintenance score.

image

image

In some way, using rehype-react is actually conceptually simpler. All other rendering is creating React elements, and passing those elements to ReactDOMServer.renderToString. When we use rehype-stringify, we are using a conversion from hast tree to html which might be different in subtle ways from how ReactDOMServer is implemented, so the html coming out of markdown could differ slightly from the html coming out of other react components.

@agarwal
Copy link
Member

agarwal commented Apr 29, 2021

Personally, I'm not too worried about transitive dependencies. Others may disagree.

@ghost
Copy link
Author

ghost commented May 1, 2021

I will switch to rehype-react soon and close this issue at that time. (Here is a branch that almost finishes this work: https://github.com/ocaml/v3.ocaml.org/tree/kw1/use-rehype-react . I can finish that branch when I get some spare time.)

@agarwal agarwal assigned ghost and rdavison and unassigned ghost May 11, 2021
@ghost
Copy link
Author

ghost commented May 12, 2021

The bindings for rehype-react could get very elaborate. For now, I would implement the simplest bindings that work and then create a follow-up issue to suggest potential improvements to the bindings.

@ghost ghost changed the title Spike: markdown to html vs markdown to react.element Implement best method of rendering markdown within a react component May 20, 2021
@ghost ghost changed the title Implement best method of rendering markdown within a react component Implement best method of rendering a markdown string within a react component May 20, 2021
@ghost ghost changed the title Implement best method of rendering a markdown string within a react component Implement rendering a markdown string within a react component May 20, 2021
@ghost
Copy link
Author

ghost commented Jun 9, 2021

This issue will remain relevant if we switch from rendering markdown during the NextJS page rendering to rendering up front in OCaml before the NextJS build (ocaml/ood#35). With OMD, the new pipeline within NextJS page rendering will be something like the following:

let res = 
  let (markdownHtmlString, markdownHtmlToc) = runOmd(markdownString)
  markdownHtmlString -> Unified.process(
    Unified.unified()
    ->Unified.use(Unified.rehypeParse)
    ->Unified.use(Unified.rehypeHighlight)
    ->Unified.use(Unified.rehype2react)
...
  let props = {
     source: res.contents,
     ...
     tableOfContents: {
       ...
       toc: markdownHtmlToc
     }
  }

@ghost
Copy link
Author

ghost commented Jun 21, 2021

PR #445 may end up forgoing the use of rehype2react.

@ghost ghost changed the title Implement rendering a markdown string within a react component Implement rendering an html AST within a react component Jun 24, 2021
@ghost
Copy link
Author

ghost commented Jun 24, 2021

It looks like rehype has been removed as well, so this issue is not relevant.

@ghost ghost closed this as completed Jun 24, 2021
@ghost ghost mentioned this issue Jun 25, 2021
4 tasks
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants