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

Remove the className prop #781

Open
4 tasks done
remcohaszing opened this issue Oct 13, 2023 · 13 comments · May be fixed by #799
Open
4 tasks done

Remove the className prop #781

remcohaszing opened this issue Oct 13, 2023 · 13 comments · May be fixed by #799
Labels
🗄 area/interface This affects the public interface 🤞 phase/open Post is being triaged manually 🧑 semver/major This is a change 💬 type/discussion This is a request for comments

Comments

@remcohaszing
Copy link
Member

Initial checklist

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.
@remcohaszing remcohaszing added 🗄 area/interface This affects the public interface 🧑 semver/major This is a change 💬 type/discussion This is a request for comments 🤞 phase/open Post is being triaged manually labels Oct 13, 2023
@wooorm
Copy link
Member

wooorm commented Oct 13, 2023

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
Copy link
Member

wooorm commented Oct 27, 2023

@remcohaszing What do you think?

@remcohaszing
Copy link
Member Author

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
Copy link
Member

wooorm commented Oct 27, 2023

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
Copy link

CHE1RON commented Nov 8, 2023

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
Copy link
Member

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
Copy link
Member

JounQin commented Nov 13, 2023

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

remcohaszing added a commit that referenced this issue Nov 15, 2023
Users should wrap the `<Markdown>` component inside a `<div>` manually
instead.

Closes #781
@remcohaszing remcohaszing linked a pull request Nov 15, 2023 that will close this issue
5 tasks
@remcohaszing
Copy link
Member Author

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
Copy link
Member

JounQin commented Nov 16, 2023

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
Copy link
Member

ChristianMurphy commented Nov 16, 2023

@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
Copy link
Member

wooorm commented Nov 16, 2023

Yeah, think that’s easiest. Removing it.

@AVGVSTVS96
Copy link

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
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🗄 area/interface This affects the public interface 🤞 phase/open Post is being triaged manually 🧑 semver/major This is a change 💬 type/discussion This is a request for comments
Development

Successfully merging a pull request may close this issue.

7 participants