Skip to content

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

Closed
internettrans wants to merge 1 commit intoDefinitelyTyped:masterfrom
internettrans:classname-string
Closed

React: allow className prop to be a boxed String object.#33548
internettrans wants to merge 1 commit intoDefinitelyTyped:masterfrom
internettrans:classname-string

Conversation

@internettrans
Copy link
Copy Markdown

@internettrans internettrans commented Mar 3, 2019

Please fill in this template.

  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Test the change in your own code. (Compile and run.)
  • Add or edit tests to reflect the change. (Run with npm test.)
  • Follow the advice from the readme.
  • Avoid common mistakes.
  • Run npm run lint package-name (or tsc if no tslint.json is present).

Select one of these and delete the others:

If changing an existing definition:

  • Provide a URL to documentation or source code which provides context for the suggested changes: See code pens in my review comments
  • Increase the version number in the header if appropriate.
  • If you are making substantial changes, consider adding a tslint.json containing { "extends": "dtslint/dt.json" }.

Comment thread types/react/index.d.ts
// Standard HTML Attributes
accessKey?: string;
className?: string;
className?: string | String;
Copy link
Copy Markdown
Author

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

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

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

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

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.

@internettrans
Copy link
Copy Markdown
Author

internettrans commented Mar 3, 2019

Note that this change cause npm run lint react to fail because the linter does not allow using Boxed primitives. Because of this, I suspect this PR might be rejected/closed, but wanted to see what you guys thought. So I still created the PR :)

@typescript-bot typescript-bot added the Popular package This PR affects a popular package (as counted by NPM download counts). label Mar 3, 2019
@typescript-bot
Copy link
Copy Markdown
Contributor

typescript-bot commented Mar 3, 2019

@joeldenning Thank you for submitting this PR!

🔔 @johnnyreilly @bbenezech @pzavolinsky @digiguru @ericanderson @tkrotoff @DovydasNavickas @onigoetz @theruther4d @guilhermehubner @ferdaber @jrakotoharisoa @pascaloliv @Hotell @franklixuefei @Jessidhia @pshrmn @threepointone - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead.

@typescript-bot
Copy link
Copy Markdown
Contributor

typescript-bot commented Mar 3, 2019

@joeldenning The Travis CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Popular package This PR affects a popular package (as counted by NPM download counts).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants