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 Definition Updates in 4.9.0: potentially incorrect #19362

Closed
ririvas opened this issue Jan 23, 2020 · 14 comments
Closed

Typescript Definition Updates in 4.9.0: potentially incorrect #19362

ririvas opened this issue Jan 23, 2020 · 14 comments
Labels

Comments

@ririvas
Copy link
Contributor

ririvas commented Jan 23, 2020

Hello,

I recently pulled the latest @material-ui/core package, version 4.9.0 and my typescript compilation process began to fail. Specifically, my custom Theme override.

The media query definitions failed.

mixins: {
            toolbar: {
                background: '#fff',
                minHeight: 36,
                '@media (min-width:0px) and (orientation: landscape)': {
                    minHeight: 24
                },
                '@media (min-width:600px)': {
                    minHeight: 48
                }
            },
        },

Perhaps we deprecated this functionality but the docs say otherwise. And the release notes don't comment on this change.
image

For this particular issue, I can track it to the change #19263 . And specifically, this file: d30e6bb#diff-353a3bd7b4e96490bd34f7442f3e08c6

Here is a codesandbox demo illustrating the type error.

@eps1lon
Copy link
Member

eps1lon commented Jan 23, 2020

Perhaps we deprecated this functionality but the docs say otherwise. And the release notes don't comment on this change.

It wasn't clear in #19263 if people actually make use of it. We never got any feedback on this via types, tests or docs contributions. Seems like didn't cover all our docs with types.

I will check out if we can safely widen the type.

@jameschensmith
Copy link

Right there with @ririvas. The fixed app bar solution using mixins doesn't even work anymore.

@rossjackson
Copy link

I have a simple setup:
image
image

That is now giving me this error:

 Overload 1 of 2, '(style: Styles<Theme, {}, "toolbarMargin" | "myDrawer">, options?: Pick<WithStylesOptions<Theme>, "flip" | "element" | "defaultTheme" | ... 
6 more ... | "classNamePrefix"> | undefined): (props?: any) => Record<...>', gave the following error.
    Argument of type '(theme: Theme) => 
{ toolbarMargin: React.CSSProperties; myDrawer: { width: number; }; }' is not assignable to parameter of type 'Styles<Theme, {}, "toolbarMargin" | "myDrawer">'.      Type '(theme: Theme) => { toolbarMargin: React.CSSProperties; myDrawer: { 
width: number; }; }' is not assignable to type 'StyleRulesCallback<Theme, {}, "toolbarMargin" | "myDrawer">'.
        Call signature return types '{ toolbarMargin: CSSProperties; myDrawer: { width: number; }; }' and 'Record<"toolbarMargin" | "myDrawer", CSSProperties | 
CreateCSSProperties<{}> | ((props: {}) => CreateCSSProperties<{}>)>' are incompatible.
          The types of 'toolbarMargin' are incompatible between these types.    
            Type 'CSSProperties' is not 
assignable to type 'CSSProperties | CreateCSSProperties<{}> | ((props: {}) => CreateCSSProperties<{}>)'.
              Type 'CSSProperties' is not assignable to type 'CreateCSSProperties<{}>'.
                Index signature is missing in type 'CSSProperties'.
  Overload 2 of 2, '(styles: Styles<Theme, {}, "toolbarMargin" | "myDrawer">, options?: Pick<WithStylesOptions<Theme>, "flip" | "element" | "defaultTheme" | ... 6 more ... | "classNamePrefix"> | undefined): (props: {}) => Record<...>', gave the following error.
    Argument of type '(theme: Theme) => 
{ toolbarMargin: React.CSSProperties; myDrawer: { width: number; }; }' is not assignable to parameter of type 'Styles<Theme, {}, "toolbarMargin" | "myDrawer">'.      Type '(theme: Theme) => { toolbarMargin: React.CSSProperties; myDrawer: { 
width: number; }; }' is not assignable to type 'StyleRulesCallback<Theme, {}, "toolbarMargin" | "myDrawer">'.  TS2769   

    15 | }
    16 | 
  > 17 | const useStyles = makeStyles((theme: Theme) => ({
       |                              ^ 
    18 |     toolbarMargin: theme.mixins.toolbar,
    19 |     myDrawer: {
    20 |         width: 20,

@ririvas
Copy link
Contributor Author

ririvas commented Jan 26, 2020

@eps1lon, let me know if I can help in anyway

@eps1lon
Copy link
Member

eps1lon commented Jan 26, 2020

@eps1lon, let me know if I can help in anyway

You can try reverting the part of #19263 that changed the type of theme.mixins.toolbar while adding a type test for it. Test would be creating a createMixings.spec.ts with a theme such as from your initial issue description. Should throw in yarn typescript and pass after you reverted that particular change.

@seandearnaley
Copy link

also experiencing this issue

@oliviertassinari
Copy link
Member

@seandearnaley What problem? Do you have reproduction?

@seandearnaley
Copy link

seandearnaley commented Jan 27, 2020

@seandearnaley What problem? Do you have reproduction?

@oliviertassinari so when I updated my @material-ui/core version to 4.9.0 I started getting type definition errors on makeStyles(). I checked the release notes and docs and I can't see any breaking changes I should make. It appears to be the same aforementioned issue on this thread. Rolling back the version to ^4.8.3 gets rid of the error.

material-ui

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

const drawerWidth = 240;

export const useStyles = makeStyles(theme => ({
  appBarSpacer: theme.mixins.toolbar,
  content: {
    padding: '10px',
    height: '100vh',
    overflow: 'auto',
    width: '100vw',
  },
  root: {
    display: 'flex',
  },
  navRoot: {
    flexGrow: 1,
  },
  navPaper: {
    padding: theme.spacing(2),
    textAlign: 'left',
    color: theme.palette.text.secondary,
  },
  paper: {
    padding: theme.spacing(2),
    display: 'flex',
    overflow: 'auto',
    flexDirection: 'column',
  },
  toolbar: {
    paddingRight: 24, // keep right padding when drawer closed
  },
  toolbarIcon: {
    display: 'flex',
    alignItems: 'center',
    justifyContent: 'flex-end',
    padding: '0 8px',
    ...theme.mixins.toolbar,
  },
  appBar: {
    zIndex: theme.zIndex.drawer + 1,
    transition: theme.transitions.create(['width', 'margin'], {
      easing: theme.transitions.easing.sharp,
      duration: theme.transitions.duration.leavingScreen,
    }),
  },
  appBarShift: {
    marginLeft: drawerWidth,
    width: `calc(100% - ${drawerWidth}px)`,
    transition: theme.transitions.create(['width', 'margin'], {
      easing: theme.transitions.easing.sharp,
      duration: theme.transitions.duration.enteringScreen,
    }),
  },
  menuButton: {
    marginRight: 36,
  },
  menuButtonHidden: {
    display: 'none',
  },
  title: {
    flexGrow: 1,
  },
  drawerPaper: {
    position: 'relative',
    whiteSpace: 'nowrap',
    width: drawerWidth,
    transition: theme.transitions.create('width', {
      easing: theme.transitions.easing.sharp,
      duration: theme.transitions.duration.enteringScreen,
    }),
  },
  drawerPaperClose: {
    overflowX: 'hidden',
    transition: theme.transitions.create('width', {
      easing: theme.transitions.easing.sharp,
      duration: theme.transitions.duration.leavingScreen,
    }),
    width: theme.spacing(7),
    [theme.breakpoints.up('sm')]: {
      width: theme.spacing(9),
    },
  },
  container: {
    paddingTop: theme.spacing(4),
    paddingBottom: theme.spacing(4),
  },
  fixedHeight: {
    height: 240,
  },
  cardContainerRoot: {
    flexGrow: 1,
    '& > *': {
      margin: theme.spacing(1),
    },
  },
  gridItem: {
    alignItems: 'center',
  },
  extendedIcon: {
    marginRight: theme.spacing(1),
  },
  fabAdd: {
    margin: '0px',
    top: 'auto',
    right: '20px',
    bottom: '20px',
    left: 'auto',
    position: 'fixed',
  },
  modal: {
    position: 'absolute',
    width: 400,
    backgroundColor: theme.palette.background.paper,
    border: '2px solid #000',
    boxShadow: theme.shadows[5],
    padding: theme.spacing(2, 4, 3),
  },
}));

@seandearnaley
Copy link

any updates on this?

@ririvas
Copy link
Contributor Author

ririvas commented Jan 30, 2020

Hi,
I forked the library and started attempting to implement what @eps1lon suggested. Hopefully, I can wrap that up tomorrow by the end of the day. Then I will issue a pull request.

I've just been going through the changes made and the thread on improving type performance. Seems like I will go for a simple revert, but I wanted to make sure I understood the context

If someone can address it quicker, please go ahead.

@seandearnaley
Copy link

Thanks @ririvas sorry about asking for updates, I'm actually in no rush I just thought it was something I may have did, or some doc I missed, thanks for working on it.

@MarcNq
Copy link

MarcNq commented Jan 31, 2020

We temporarily solved the issue by casting theme.mixins.toolbar to Material-UI's CSSProperties instead of React.CSSProperties:

import {makeStyles} from '@material-ui/core/styles'
import {CSSProperties} from '@material-ui/styles'

const useStyles = makeStyles((theme: Theme) => ({
  root: {
    toolbar: theme.mixins.toolbar as CSSProperties,
  }
}))

@eps1lon
Copy link
Member

eps1lon commented Feb 1, 2020

Closed with #19491

@eps1lon eps1lon closed this as completed Feb 1, 2020
@gavindang2911
Copy link

const useStyles = makeStyles(() => {
  const style: StyleRules = {
    root: {
      fontWeight: 600,
      fontSize: '30px',
    },
  };
  return style;
});

Using StyleRules should work

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

No branches or pull requests

8 participants