React: allow className prop to be a boxed String object.#33548
React: allow className prop to be a boxed String object.#33548internettrans wants to merge 1 commit intoDefinitelyTyped:masterfrom
Conversation
| // Standard HTML Attributes | ||
| accessKey?: string; | ||
| className?: string; | ||
| className?: string | String; |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Perhaps the alternative is to have the types of kremling return a value that is & stringed instead of being a subtype of String.
There was a problem hiding this comment.
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.There was a problem hiding this comment.
Wrong operator -- & (intersection, both at the same time), not | (either one of them).
There was a problem hiding this comment.
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.
|
Note that this change cause |
|
@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 If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead. |
|
@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! |
Please fill in this template.
npm test.)npm run lint package-name(ortscif notslint.jsonis present).Select one of these and delete the others:
If changing an existing definition:
tslint.jsoncontaining{ "extends": "dtslint/dt.json" }.