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

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

Merged
merged 1 commit into from
Jun 15, 2020
Merged

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

merged 1 commit into from
Jun 15, 2020

Conversation

vismu
Copy link
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
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
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
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
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
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
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
Member

kof commented Jun 15, 2020

Alright, merged!

@vismu
Copy link
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.

None yet

4 participants