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

[Typescript] Invalid createStyles definition (v4 beta) #15532

Closed
2 tasks done
ljvanschie opened this issue Apr 30, 2019 · 8 comments · Fixed by #15547
Closed
2 tasks done

[Typescript] Invalid createStyles definition (v4 beta) #15532

ljvanschie opened this issue Apr 30, 2019 · 8 comments · Fixed by #15547
Labels
good first issue Great for first contributions. Enable to learn the contribution process. package: material-ui Specific to @mui/material typescript

Comments

@ljvanschie
Copy link
Contributor

  • This is not a v0.x issue.
  • I have searched the issues of this repository and believe that this is not a duplicate.

It seems the typings for createStyles are invalid in v4 beta 0. As you can see in the following snippet, Typescript will accept:

import { createStyles } from '@material-ui/core';

but runtime, this will throw an error (_core.createStyles is not a function):

https://codesandbox.io/s/wqqzkjvyx5

It seems like an attempt has been made to forward the definition to the styles package, but if I'm not missing something, this does not work.

@eps1lon eps1lon added good first issue Great for first contributions. Enable to learn the contribution process. package: material-ui Specific to @mui/material typescript labels Apr 30, 2019
@eps1lon
Copy link
Member

eps1lon commented Apr 30, 2019

For convenience we should just forward this to @material-ui/styles. The commented warning can be removed as long as we have 2 possible imports for a styling solution.

@eps1lon
Copy link
Member

eps1lon commented Apr 30, 2019

Aside: importing from @material-ui/core/styles works. Path imports should only be used for 1st and 2nd level e.g. import { withStyles } from '@material-ui/core' or import { withStyles } from '@material-ui/core/styles' but not import withStyles from '@material-ui/core/styles/withStyles'.

@ljvanschie
Copy link
Contributor Author

Thanks for the quick response 👍

@oliviertassinari
Copy link
Member

@eps1lon On a different point, I was under the assumption that createStyles is no longer needed for withStyles and soon for makeStyles. Should we reflect this fact in the upgrade guide?

@eps1lon
Copy link
Member

eps1lon commented Apr 30, 2019

It was never needed for either one. It was and is a convenience helper.

@oliviertassinari
Copy link
Member

@eps1lon Rereading #15390 (comment), the important word is "immediate". This convenience helper is still needed.

@eps1lon
Copy link
Member

eps1lon commented Apr 30, 2019

@eps1lon Rereading #15390 (comment), the important word is "immediate". This convenience helper is still needed.

Correct. The main issue is type widening

const someObject = { foo: 'literal' }; // typeof someObject = { foo: string }
takeSomeObject(someObject); // throws if it expects { foo: 'literal' | 'anotherLiteral' }
takeSomeObject({ foo: 'someLiteral' }): // works since typescript doesn't infer a type. it just checks if the expected type matches the given type

@eps1lon
Copy link
Member

eps1lon commented Apr 30, 2019

So everybody who wants to create an intermediary variable for the styles(creator|object) still needs the helper.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Great for first contributions. Enable to learn the contribution process. package: material-ui Specific to @mui/material typescript
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants