Skip to content
This repository has been archived by the owner on Dec 13, 2018. It is now read-only.

Typescript fails on Div and Button elements, if you give the onClick attribute a function as argument #353

Closed
Vanluren opened this issue Nov 2, 2017 · 13 comments

Comments

@Vanluren
Copy link

Vanluren commented Nov 2, 2017

  • glamorous version: 4.5.0
  • glamor version: 2.20.40
  • react version: 16.0.0
  • typescript version: 2.6.1, 2.53

Relevant code.

<Button css={ buttonStyle } onClick={() => Raven.lastEventId() && Raven.showReportDialog()}>
File Report
</Button>

What you did:

I tried to give a glamorous Div and a Button Element an onClick attribute with a Function as argument

What happened:

The following error is displayed:

Error:(30, 12) TS2322: Type '{ css: CSSProperties; onClick: () => void | ""; children: string; }' is not assignable to type 'IntrinsicAttributes & CSSProperties & ExtraGlamorousProps & HTMLProps<HTMLButtonElement> & { chil...'.
  Type '{ css: CSSProperties; onClick: () => void | ""; children: string; }' is not assignable to type 'CSSProperties'.
    Property 'onClick' is incompatible with index signature.
      Type '() => void | ""' is not assignable to type 'string | number | SingleOrArray<CSSPropertiesCompleteSingle, "alignContent" | "alignItems" | "ali...'.
        Type '() => void | ""' is not assignable to type 'ReactElement<any>'.

Reproduction:

I was not able to reproduce the problem using Stackblitz, but if you clone this repo

The issue is present in the src/index.tsx file

Problem description:

When the glamorous 'Button' element is given an the onClick attribute, typescript fails

Suggested solution:

If you change the CSSPropertiesLossy [glamorous/typings/css-properties.d.ts(l.1921)] interface to allow functions as a type, the error is not displayed, like so:

export interface CSSPropertiesLossy {
  [propertyName: string]:
    | string
    | number
    | CSSPropertiesComplete
    | undefined
    | Array<CSSPropertiesComplete[keyof CSSPropertiesComplete]>
    | CSSPropertiesLossy
    | React.ReactChild
      | Function
}
@Ailrun
Copy link
Contributor

Ailrun commented Nov 2, 2017

Hm... that's weird. I will investigate this.

@Vanluren
Copy link
Author

Vanluren commented Nov 29, 2017

Any updates on this?

@Ailrun
Copy link
Contributor

Ailrun commented Nov 29, 2017

Actually,

const x: : CSSProperties | ExtraGlamorousProps | React.HTMLProps<HTMLButtonElement> {
    css: buttonStyle,
    onClick: () => console.log('Hello')
}

<Button {...x} >FileError</Button>

is works in your example repo, so the problem may be in React typing (or TS itself), not in Glamorous...

@Ailrun
Copy link
Contributor

Ailrun commented Nov 29, 2017

Glamorous Button has type of

React.SFC<CSSProperties | ExtraGlamorousProps | React.HTMLProps<HTMLButtonElement>>

@Vanluren
Copy link
Author

Vanluren commented Dec 5, 2017

So what does this mean exactly? Does it mean, that if you want to attach an onClick function to a button element, this can only be done with a with the object syntax you have shown in your previous reply?

@Ailrun
Copy link
Contributor

Ailrun commented Dec 5, 2017

@Vanluren Sorry for delay. I'm little busy now days...
I notice that type with | (or Union type) is my local one where the real type use & (Intersection Type).
I'm really sorry for that. For this case, it's not a problem of typescript or react typing. It just try to assign () => void (or any Function) to

    (string
    | number
    | CSSPropertiesComplete
    | undefined
    | Array<CSSPropertiesComplete[keyof CSSPropertiesComplete]>
    | CSSPropertiesLossy
    | React.ReactChild)
   & HTMLProps<HTMLButtonElement>['onClick']

and of course this fails. Furthermore, instantiating this type is almost impossible.

So your suggestion is one solution for this problem, but... what I'm afraid of is adding Function to CSSProperties making CSSProperties too weak.

@luke-john How about your opinion? Is adding Function to CSSPropertiesLossy (and therefore added to CSSProperties looks OK?

@luke-john
Copy link
Collaborator

Thank @Vanluren for the detailed issue and @Ailrun for looking into this already. I really enjoy how friendly and helpful people are on this project 💯.

Apologies for my lack of attention here, having a look at this I think this would be solved by having the named built in glamorous components use CSSPropertiesComplete rather than CSSProperties. I suspect some tests would start failing though due to reliance on unexpected behaviour.

Re looking at the types though after some time away, there's a few things that could be simplified around the place 🙂.

I won't have time to look at any of this until next week at the earliest as I'm off on holidays 🍷☀️.

As a temporary measure to get typesafety without asserting to an any you could do something like follows.

import { CSSProperties, CSSPropertiesComplete, ExtraGlamorousProps,  Button } from 'glamorous';

type TmpButton = React.StatelessComponent<
  React.HTMLProps<HTMLButtonElement> & ExtraGlamorousProps & CSSPropertiesComplete
>

const TmpButton = Button as TmpButton

...
<Button css={ buttonStyle } onClick={() => Raven.lastEventId() && Raven.showReportDialog()}>
    File Report
</Button>

@matt-erhart
Copy link

When this is fixed, glamorous might be the best css in ts package I've seen.

@JReinhold
Copy link

@luke-john is there anything I can assist you with on this matter? :) We are unfortunately also experiencing the same issue with the Input component.

@Ailrun
Copy link
Contributor

Ailrun commented Jan 8, 2018

@JReinhold Actually, this occurs in every component, using it with the properties that requires other than CSSPropertiesLossy allows.

@luke-john
Copy link
Collaborator

Apologies for the lack of action on this issue.

This may be solved by #372 (assuming that works 😄).

@GeeWee
Copy link

GeeWee commented Mar 21, 2018

We've just upgraded to the latest glamorous and #372 does not solve the issue :(

@kentcdodds
Copy link
Contributor

This project is now in an unmaintained status. Please see the README for more information.

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

No branches or pull requests

7 participants