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

Incompatibilities with @types/react #4

Closed
pelotom opened this issue Feb 3, 2018 · 9 comments
Closed

Incompatibilities with @types/react #4

pelotom opened this issue Feb 3, 2018 · 9 comments

Comments

@pelotom
Copy link
Collaborator

pelotom commented Feb 3, 2018

I was taking a look at what it would take to get @types/react to use csstype instead of its own internal definition for CSSProperties. So far I've run into the following issues:

  1. CSSProperties uses numeric literals for fontWeight:

    fontWeight?: CSSWideKeyword | "normal" | "bold" | "bolder" | "lighter" | 100 | 200 | 300 | 400 | 500 | 600 | 700 | 800 | 900;
    

    whereas csstype uses all string literals:

    type FontWeightProperty = All | "900" | "bold" | "bolder" | "lighter" | "100" | "200" | "normal" | "400" | "500" | "600" | "700" | "800" | "300";
    

    Could this be changed to use numeric literals?

  2. SVG properties such as fillOpacity, strokeOpacity and strokeWidth appear to be missing.

@frenic
Copy link
Owner

frenic commented Feb 3, 2018

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.

@frenic
Copy link
Owner

frenic commented Mar 14, 2018

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 stop-color property.

@frenic
Copy link
Owner

frenic commented Mar 21, 2018

I just did a quick test and it seems like React.CSSProperties are now compatible with csstype.

const x: CSS.Properties<string | number> = {} as React.CSSProperties; // OK

@pelotom
Copy link
Collaborator Author

pelotom commented Mar 21, 2018

It's still not quite compatible when assigning style literals because of the missing SVG properties. I have a branch of @types/react where I define type CSSProperties = CSS.Properties<string | number> here:

https://github.com/pelotom/DefinitelyTyped/tree/react-csstype

Running npm run lint react produces the following errors:

Error: /Users/tom/code/DefinitelyTyped/types/react/test/cssProperties.tsx:41:49
ERROR: 41:49  expect  TypeScript@next compile error:
Type '{ fillOpacity: number; }' is not assignable to type 'Properties<Key>'.
  Object literal may only specify known properties, and 'fillOpacity' does not exist in type 'Properties<Key>'.
ERROR: 44:51  expect  TypeScript@next compile error:
Type '{ strokeOpacity: number; }' is not assignable to type 'Properties<Key>'.
  Object literal may only specify known properties, and 'strokeOpacity' does not exist in type 'Properties<Key>'.
ERROR: 47:49  expect  TypeScript@next compile error:
Type '{ strokeWidth: string; }' is not assignable to type 'Properties<Key>'.
  Object literal may only specify known properties, and 'strokeWidth' does not exist in type 'Properties<Key>'.

One escape hatch would be to just allow any arbitrary additional properties:

export interface CSSProperties extends CSS.Properties<string | number> {
  [k: string]: any;
}

which is what @types/react is already doing. TypeStyle takes the approach of exhaustively enumerating all properties, including the SVG ones:

https://github.com/typestyle/typestyle/blob/0ba077e53afdcd15ca6e9e26c17e7f1be8de5343/src/types.ts#L1871-L1875

(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.

@frenic
Copy link
Owner

frenic commented Mar 21, 2018

Sure, I didn't mean fully compatible. But at least there's no mismatch on common properties with csstype@2.0.0 as the previous version had on justifyContent.

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 Properties interface. It would be nice if you could do something like this to include properties that only applies to certain elements:

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 Properties would of course be at least something and a good start.

@pelotom
Copy link
Collaborator Author

pelotom commented Mar 22, 2018

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.

@frenic
Copy link
Owner

frenic commented Mar 22, 2018

I hear you 👍 SVG properties should at least be separated but included in Properties like vendor properties.

@pelotom
Copy link
Collaborator Author

pelotom commented Apr 3, 2018

FYI I've opened a PR on @types/react to use csstype as-is with a string index fallback: DefinitelyTyped/DefinitelyTyped#24688

@frenic
Copy link
Owner

frenic commented Apr 3, 2018

Thank you! I'll do my best with SVG properties as soon as possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants