Skip to content

React: allow className prop to be a boxed String object. #33548

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.

Sign up for GitHub

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

Closed
internettrans wants to merge 1 commit into from
Closed

React: allow className prop to be a boxed String object. #33548

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions types/react/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1423,7 +1423,7 @@ declare namespace React {

// Standard HTML Attributes
accessKey?: string;
className?: string;
className?: string | String;
Copy link
Copy Markdown
Author

@internettrans internettrans Mar 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although uncommon (and admittedly weird), React does support passing in a boxed String for the className prop (and for other dom properties, too). It works because react is simply passing the className along to the DOM, which supports passing in a boxed String as the className.

React example: https://codepen.io/joeldenning/pen/oVxmQL
DOM example: https://codepen.io/joeldenning/pen/qvZgLb

The thing that caused me to make this change is that the kremling css library passes boxed String objects to className to allow for a cool chained API for constructing classNames. See https://kremling.js.org/api/always-maybe-and-toggle.html for examples. Afaik, you cannot add methods to a primitive string, and adding to the entire String prototype is undesireable. So the kremling implementation is to return a boxed String object that has some extra methods on it (always, maybe, and toggle)

Copy link
Copy Markdown
Member

@Jessidhia Jessidhia Mar 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps the alternative is to have the types of kremling return a value that is & stringed instead of being a subtype of String.

Copy link
Copy Markdown
Author

@internettrans internettrans Mar 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried that but I still get a compilation error. Typescript is too smart to be fooled into letting something that has to be a string be tricked by a maybe string maybe KremlingString

typescript-test ❯ ./node_modules/.bin/tsc
index.tsx:4:6 - error TS2322: Type 'string | KremlingString' is not assignable to type 'string'.
  Type 'KremlingString' is not assignable to type 'string'.

4 <div className={always('foo')} />
       ~~~~~~~~~

  node_modules/@types/react/index.d.ts:1426:9
    1426         className?: string;
                 ~~~~~~~~~
    The expected type comes from property 'className' which is declared here on type 'DetailedHTMLProps<HTMLAttributes<HTMLDivElement>, HTMLDivElement>'


Found 1 error.

Copy link
Copy Markdown
Member

@Jessidhia Jessidhia Mar 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong operator -- & (intersection, both at the same time), not | (either one of them).

Copy link
Copy Markdown
Author

@internettrans internettrans Mar 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops -- somehow missed that. That worked! Thanks for help. I'm new to typescript and didn't know what | or & were, or what the difference is. Thanks for teaching me.

contentEditable?: boolean;
contextMenu?: string;
dir?: string;
Expand Down Expand Up @@ -2174,7 +2174,7 @@ declare namespace React {
interface SVGAttributes<T> extends DOMAttributes<T> {
// Attributes which also defined in HTMLAttributes
// See comment in SVGDOMPropertyConfig.js
className?: string;
className?: string | String;
color?: string;
height?: number | string;
id?: string;
Expand Down