Remove the `className` prop

[remcohaszing]

Initial checklist

• [X] I read the support docs
• [X] I read the contributing guide
• [X] I agree to follow the code of conduct
• [X] I searched issues and couldn’t find anything (or linked relevant results below)

Problem

Typically a className prop in React adds a class name to the element it creates. For react-markdown this is not the case. Instead, it creates a wrapper that gets the class name. This is a fairly useless feature, because the user can wrap the content in a wrapper component themselves. That even gives the user more control.

In other words, these two are equivalent:

import Markdown from 'react-markdown'

export function Component({ children }) {
  return (
    <Markdown className="some-class">
      {children}
    </Markdown>
  )
}

import Markdown from 'react-markdown'

export function Component({ children }) {
  return (
    <div className="some-class">
      <Markdown>
        {children}
      </Markdown>
    </div>
  )
}

Solution

Deprecate the className prop, and remove it in then next semver major release.

Alternatives

• Do nothing.
• Don’t deprecate it.
• Make a semver major release just for this.

[wooorm]

a) is it really worth the churn to remove a nice to have feature that isn’t super useful but well, folks might want, and also people will already have, do we really need to break peoples code b) we always generate a block. Generating a div around blocks is fine, doesn’t cause a shift.

Sure it’s quite useless. But it isn’t bad?

One more alternative: accept all IntrinsicAttributes['div'] too.

[wooorm]

@remcohaszing What do you think?

[remcohaszing]

I would flip the question: If we didn’t support the className prop yet, would it be worth adding it, if it requires doing something unexpected (adding a wrapper element)?

This behaviour is documented, but typically a className prop forwards it to the top-level component, not insert one additionally.

The alternative of having the user wrap <Markdown /> inside a <div /> manually is much more flexible. They can add any props they want to the <div />, or even use an entirely different element.

I don’t think we should remove of right away, but I think it would be nice to remove in a next major version.

[wooorm]

I’m not really opposed. But I feel like the churn/discussions/etc just doesn’t matter much. There’s a lot to maintain. Feel free to PR a comment: // To do: deprecate next major.

[CHE1RON]

Deprecating on next major or not, for the time being passing all IntrinsicAttributes['div'] grants more flexibility while not breaking anything for people (not) using className!

[ChristianMurphy]

I'm of a similar mind to @remcohaszing, I see a mapping of className or other <div> related props as muddling the top level API. Users adding their own wrapper is more flexible and clear.

I'd lean against adding even more <div> props before a release removing support. We want to minimize the amount of attributes users will need to change, not add more.

Feel free to PR a comment

Would we also want to add a devlop deprecation notice? (https://github.com/wooorm/devlop)

[JounQin]

Personally I prefer fewer nested depth, so I would like to add all props instead of removing the support.

[remcohaszing]

The problem with adding more props is, how far do you want to go? <div> supports tens (hundreds? Infinity including data-*?) of props. Why <div>, and not <main>, or a custom component? Should there be an as prop?

All of these choices complicate the API. Also there was simply no other way before the introduction of fragments / returning arrays. Removing the prop and promoting composition simplifies it.

[JounQin]

All props can be supported via simple {...props}, and for the tag name, div vs main doesn't change anything, it's just a choice. Or even it can be supported to be changed while I don't think it makes any sense.

Or even the user can use React.cloneElement easily if they really want to hack.

Actually the user can just don't pass any HTML attributes with a wrapper by themselves.

[ChristianMurphy]

@JounQin there seems to be two different discussions here:

1. what is possible in code
2. what makes sense for the API

I completely agree that is it possible, though code to support all attributes and switching HTML tags. I also agree that flatter HTML/JSX is generally preferable.

I respectfully disagree that react-markdown should provide a magic root level tag insertion feature. The focus of the library is to render markdown to react. Features that are unrelated or tangential to rendering markdown, confuse the API, and should be avoided/removed. In my mind adding random <div> attributes on top of all the markdown related attributes falls in the category of confusing/distracting.

[wooorm]

Yeah, think that’s easiest. Removing it.

[AVGVSTVS96]

Yeah, think that’s easiest. Removing it.

I think it's best to leave it as is. It just doesn't seem like a pressing concern and will require users to change their code, all for something that isn't truly important.

Let's leave this as is, I think we should minimize breaking changes, especially when it's something as benign as this. It's a nice-to-have without negative consequences, no harm.

[ivanzusko]

Agree with @AVGVSTVS96. It is better to keep it, at least for now. Maybe when the next necessary breaking change will be introduced - the className can be deprecated alongside