Skip to content

fix(react-jss): fix react-jss theme types#1349

Merged
kof merged 1 commit intocssinjs:masterfrom
vismu:master
Jun 15, 2020
Merged

fix(react-jss): fix react-jss theme types#1349
kof merged 1 commit intocssinjs:masterfrom
vismu:master

Conversation

@vismu
Copy link
Copy Markdown
Contributor

@vismu vismu commented Jun 4, 2020

What would you like to add/fix?

The problem is that my app Theme interface didn't match to unknown theme type.
So I think any type would be better in this case.

@vismu vismu requested a review from HenriBeck as a code owner June 4, 2020 15:57
@vismu
Copy link
Copy Markdown
Contributor Author

vismu commented Jun 10, 2020

Hello, dear colleagues!
Any updates here?
I still have an issue with matching Theme interface to unknown type.
image
Maybe any advices how to do it in a right way.

@kof
Copy link
Copy Markdown
Member

kof commented Jun 10, 2020

Is this the best way to do it in ts? Is user able to define a strict theme? It sounds like it should be a generic but I only use flowtype, so I have no idea
cc @moshest can you advise us here pls?

@vismu
Copy link
Copy Markdown
Contributor Author

vismu commented Jun 15, 2020

Is this the best way to do it in ts? Is user able to define a strict theme? It sounds like it should be a generic but I only use flowtype, so I have no idea
cc @moshest can you advise us here pls?

Of course we can add second generic for WithStylesProps to specify theme object in more strict way.

interface WithStylesProps<S extends Styles | ((theme: T) => Styles), T> {
  classes: Classes<S extends ((theme: T) => Styles) ? keyof ReturnType<S> : keyof S>
}

But, actually, we don't need this Theme generic to calculate our classes result.
So it will be a little bit overhead and do the usage more annoying.

interface Props extends WithStylesProps<typeof styles, Theme> {}

vs

interface Props extends WithStylesProps<typeof styles> {}

@kof
Copy link
Copy Markdown
Member

kof commented Jun 15, 2020

Mb I misunderstand something, is user able to specify the theme type right now and let ts show this type everywhere it is used?

@vismu
Copy link
Copy Markdown
Contributor Author

vismu commented Jun 15, 2020

In my app I have a Theme interface and use it while defining of dynamic styles for component.

const styles = (theme: Theme) => {...}

@moshest
Copy link
Copy Markdown
Member

moshest commented Jun 15, 2020

I agree.

We should change theme type to any:

//                                                    ↧ this one
interface WithStylesProps<S extends Styles | ((theme: any) => Styles)> {
  classes: Classes<S extends ((theme: any) => Styles) ? keyof ReturnType<S> : keyof S>
}

@kof kof merged commit dded593 into cssinjs:master Jun 15, 2020
@kof
Copy link
Copy Markdown
Member

kof commented Jun 15, 2020

Alright, merged!

@vismu
Copy link
Copy Markdown
Contributor Author

vismu commented Jun 16, 2020

Alright, merged!

Great! Can you tell me how frequently new versions of react-jss are published.

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

Successfully merging this pull request may close these issues.

4 participants