Refactor docs to use createRoot

[tris203]

Initial checklist

• I read the support docs
• I read the contributing guide
• I agree to follow the code of conduct
• I searched issues and couldn’t find anything (or linked relevant results below)
• If applicable, I’ve added docs and tests

Description of changes

Rewrite of the ReadMe to use more traditional functional components

Also rewrite the syntax highlighting example to avoid nested components, shadowing and generally make it more readable

[codecov-commenter]

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (2245c64) 100.00% compared to head (6779f4a) 100.00%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff            @@
##              main      #779   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            3         3           
  Lines         1354      1354           
  Branches       113       113           
=========================================
  Hits          1354      1354           

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

[ChristianMurphy]

Welcome @tris203!
Thanks for taking some time and consideration looking at the docs.

The current docs are intentional in having everything needed for a runnable example in that single file.
Hence why react-dom and the render function are included.

traditional functional components

Class components are more traditional. 🙂
Though functional components are increasingly popular.
I'm not sure either is needed here though, JSX can exist outside a component as well, adding a pass-through wrapper adds complexity rather than removing it.

Also rewrite the syntax highlighting example to avoid nested components, shadowing and generally make it more readable

Having the inline function is intended here, when used with TypeScript, it allows the props to be inferred rather than needing to be explicitly typed.

[tris203]

Welcome @tris203! Thanks for taking some time and consideration looking at the docs.

The current docs are intentional in having everything needed for a runnable example in that single file. Hence why react-dom and the render function are included.

traditional functional components

Class components are more traditional. 🙂 Though functional components are increasingly popular. I'm not sure either is needed here though, JSX can exist outside a component as well, adding a pass-through wrapper adds complexity rather than removing it.

Also rewrite the syntax highlighting example to avoid nested components, shadowing and generally make it more readable

Having the inline function is intended here, when used with TypeScript, it allows the props to be inferred rather than needing to be explicitly typed.

Traditional was probably the wrong word, I probably meant the opposite in modern.

Understand your comments, I just found specifically in the Syntax highlighting example when I came to use it, I found it clearer written as I had it.

But I see your point about all in one file, although I would counter point that the use case is more likely to be as part of a larger application and therefore there might be some benefit to them being this way in the docs?

Perhaps, I could rewrite this into an additional page of "Usage within a existing React Application", if you think there would be benefit to that?

Thanks for your consideration of my suggestions!

[remcohaszing]

I like these changes. Users will typically use this react-markdown inside a component in an existing React application, not render them directly. So this is closer to how it’s typically used.

The current docs are intentional in having everything needed for a runnable example in that single file.
Hence why react-dom and the render function are included.

I do like the idea of a runnable example idea. We could have both though. THe example could define a component App, then <App /> can be rendered.

render() is deprecated though. Better use createRoot().

Also rendering directly in the body is an anti-pattern. It’s considered a good practice to create a separate DOM node to render a React app.

const domNode = document.createElement('div');
document.body.appendChild(domNode);
const root = createRoot(domNode); 
root.render(<App />);

[tris203]

const domNode = document.createElement('div');
document.body.appendChild(domNode);
const root = createRoot(domNode); 
root.render(<App />);

Great idea @remcohaszing , happy to make these changes if people agree.

This would also bring it inline with React 18 best practices

[ChristianMurphy]

I do like the idea of a runnable example idea. We could have both though. THe example could define a component App, then can be rendered.

I'm ambivalent on this, adding a wrapper component, just because of convention, feels like adding unnecessary indirection to me.

render() is deprecated though. Better use createRoot().

Good idea!

Also rendering directly in the body is an anti-pattern. It’s considered a good practice to create a separate DOM node to render a React app.

I understand and agree, with a caveat.
If we have a different/custom root, then we'd need to define and share the HTML file as well.
Versus body works with any document.
It feels like a custom identifier would add a fair bit of noise for the sake of convention.

[tris203]

Dan Abramov's comment here explains the reason to avoid rendering into body directly is to avoid collisions with other scripts

facebook/create-react-app#1568 (comment)

I guess the decision to be made is should the docs focus on a standalone example or an example for integrating into a larger app.

Or do we try and strike a balance of both, which evidently it's harder than it seems!

[wooorm]

How is export default function App() { return .... } a better example? Of course you shouldn’t render in the body, but it works. Beginners see something happening. And they can target a specific node if they have that and want to render in it. How to properly use react is documented in the react docs.
But an export default is not always usable, it doesn’t work in some cases.

Practically though, most people will understand both. And know that they don’t actually need it. They can copy the <Markdown>...</Markdown> part and place it where they want.

[wooorm]

Perhaps, I could rewrite this into an additional page of "Usage within a existing React Application", if you think there would be benefit to that?

What would you write in this?
That might be useful.
I can also see it useful to have a bit on how to type components. What props will be!

[wooorm]

Also rendering directly in the body is an anti-pattern. It’s considered a good practice to create a separate DOM node to render a React app.

const domNode = document.createElement('div');
document.body.appendChild(domNode);
const root = createRoot(domNode); 
root.render(<App />);

That seems really verbose to add everywhere. I don’t think that improves the situation for readers.

[tris203]

How is export default function App() { return .... } a better example? Of course you shouldn’t render in the body, but it works. Beginners see something happening. And they can target a specific node if they have that and want to render in it. How to properly use react is documented in the react docs.
But an export default is not always usable, it doesn’t work in some cases.

Practically though, most people will understand both. And know that they don’t actually need it. They can copy the <Markdown>...</Markdown> part and place it where they want.

I agree for the simpler examples

My main experience was coming in as user finding this thing that would render markdown with syntax highlighting and the example was a initially intimidating with nested components, shadowed variables, children and regexs all nested as props.

Where as breaking the highlighter into a seperate function makes it more readable, atleast for me, and demonstrates better that the code tag is being passed into that function etc and inspires ideas of how it could be reused with other tags, plugins etc

My main focus was just that the example should be in a format that most people who find the project would want to use it in, and I thought that is more likely to be nested as some part of another project, a blog, some other docs etc and as such a modified example could easier to follow for those people

[remcohaszing]

I see both valid arguments for having a runnable example and having an example that closer resembles actual usage. I think I’d go closer to actual usage if I wrote it myself, but this is subjective. I think users should be able to implement this in their own code based on either kind of example.

Also rendering directly in the body is an anti-pattern. It’s considered a good practice to create a separate DOM node to render a React app.

I understand and agree, with a caveat. If we have a different/custom root, then we'd need to define and share the HTML file as well. Versus body works with any document. It feels like a custom identifier would add a fair bit of noise for the sake of convention.

const domNode = document.createElement('div');
document.body.appendChild(domNode);
const root = createRoot(domNode); 
root.render(<App />);

That seems really verbose to add everywhere. I don’t think that improves the situation for readers.

I good to show an example that’s more correct, but I see why you prefer using document.body here.

I do strongly believe it’s good to not use a deprecated API, even if it’s replacement is a bit more verbose. It doesn’t have to be that much more verbose.

- import ReactDom from 'react-dom'
+ import { createRoot } from 'react-dom/client';

- ReactDOM.render(<App />, document.body)
+ createRoot(document.body).render(<App />)

[tris203]

I have made some changes to cover the suggestions made

[wooorm]

• Here’s some style suggestions you can apply to match the project
• Q: I don’t think it’s good to both a) expose, b) use. Practically I recommend dropping export default from all these cases. I think it’s clear enough then that folks can export it if they want (likely their framework already starts with something like that), but that the code also runs as-is.
  I’d like to hear more opinions on this though. I don’t think it is an improvement. I think createRoot(document.body).render(<Markdown>...</Markdown>) is better.

[tris203]

formatting changes have been made and export removed

My opinion is that a component as a whole is more readable,, rather than nesting the whole thing inside a render function but again open to others opinions

[wooorm]

@ChristianMurphy, @remcohaszing what do you think of my latest Q?

[wooorm]

Ping!

[ChristianMurphy]

I’d like to hear more opinions on this though. I don’t think it is an improvement. I think createRoot(document.body).render(<Markdown>...</Markdown>) is better.

I'd agree.
We're introducing how to use the <Markdown> element, not create a comprehensive lesson on structuring a React project.
Directly rendering is the shortest path to showing how to use the element.
We could link to https://react.dev/ if there is a strong concern people won't understand how to create a basic function component.

[remcohaszing]

I agree there’s not need to export App, but that has been removed already. :)

I’m not really convinced using <App /> is either more or less clear. I’m fine with either option.

[tris203]

I’d like to hear more opinions on this though. I don’t think it is an improvement. I think createRoot(document.body).render(<Markdown>...</Markdown>) is better.

I'd agree.
We're introducing how to use the <Markdown> element, not create a comprehensive lesson on structuring a React project.
Directly rendering is the shortest path to showing how to use the element.
We could link to https://react.dev/ if there is a strong concern people won't understand how to create a basic function component.

I agree with the simpler components. The issue i had with the existing docs was specifically with the syntax highlighting example.

This was the main reason for adjustment and where I see the most improvement personally. However, it is obviously best to change the others too for consistency.

[wooorm]

Thanks for your work! I picked the createRoot from your PR.

The issue i had with the existing docs was specifically with the syntax highlighting example.

For the future, I recommend just PRing what you really care about.
Splitting different things into separate PRs.
I think it’s great to use createRoot, thanks!

I don’t think App is an improvement.
I don’t think using code: generateCodeBlock and another generateCodeBlock function is
an improvement, for one it is untyped, and mainly I think it’s just different not really better.