-
Notifications
You must be signed in to change notification settings - Fork 73
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
Incompatibilities with @types/react #4
Comments
Nice catch, I'll fix that. I guess keywords containing only digits should be converted to a numeric literal instead. SVG properties are missing in https://github.com/mdn/data. Here's an issue about that. |
Here's an index of SVG properties. But there's no raw data to use as far as I can see. Either way, there's a complexity described here with SVG properties that some of them only apply to a very limited group of elements. Some of them only apply to one single element like the |
I just did a quick test and it seems like const x: CSS.Properties<string | number> = {} as React.CSSProperties; // OK |
It's still not quite compatible when assigning style literals because of the missing SVG properties. I have a branch of https://github.com/pelotom/DefinitelyTyped/tree/react-csstype Running
One escape hatch would be to just allow any arbitrary additional properties:
which is what (By hand? 🤷♂️) What do you think is the best approach? I personally would prefer exhaustiveness, but I can understand that that's annoying to maintain if it has to be done manually. |
Sure, I didn't mean fully compatible. But at least there's no mismatch on common properties with [email protected] as the previous version had on I agree that the missing SVG properties is a problem because the whole idea is to have as perfect types as possible so index signatures aren't needed. I was thinking about writing them manually since they are not that many SVG properties. I don't see that as a problem. But I'm not sure how to include them in the const style: CSS.Properties<Element> = {
fill: 'red', // OK
stopColor: 'green', // OK
};
const htmlStyle: CSS.Properties<HTMLElement> = {
fill: 'red', // Error
stopColor: 'green', // Error
};
const svgStyle: CSS.Properties<SVGElement> = {
fill: 'red', // OK
stopColor: 'green', // OK
};
const svgStopStyle: CSS.Properties<SVGStopElement> = {
fill: 'red', // OK
stopColor: 'green', // OK
};
const svgRectStyle: CSS.Properties<SVGRectElement> = {
fill: 'red', // OK
stopColor: 'green', // Error
}; Now the first generic argument is occupied. But in theory. This would be possible with TypeScript conditional types that will be released in v2.8. But the problem is that Flow doesn't have any feature like that. Including them all under |
I’m as excited to apply conditional types to every problem as the next person, but I think this may be a case of diminishing returns on investment 🙂 I would guess that 95% of downstream libraries don’t care to distinguish between the properties that are valid for one type of element vs another... the most pressing need is for a complete yet finite union of possible properties and their corresponding value types. |
I hear you 👍 SVG properties should at least be separated but included in |
FYI I've opened a PR on |
Thank you! I'll do my best with SVG properties as soon as possible. |
I was taking a look at what it would take to get
@types/react
to usecsstype
instead of its own internal definition forCSSProperties
. So far I've run into the following issues:CSSProperties
uses numeric literals forfontWeight
:whereas
csstype
uses all string literals:Could this be changed to use numeric literals?
SVG properties such as
fillOpacity
,strokeOpacity
andstrokeWidth
appear to be missing.The text was updated successfully, but these errors were encountered: