Skip to content

makeStyles overload breakage with TypeScript 4.1 #23627

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

Closed
2 tasks done
danvk opened this issue Nov 20, 2020 · 11 comments · Fixed by #23633
Closed
2 tasks done

makeStyles overload breakage with TypeScript 4.1 #23627

danvk opened this issue Nov 20, 2020 · 11 comments · Fixed by #23633
Labels
bug 🐛 Something doesn't work package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5. typescript

Comments

@danvk
Copy link

danvk commented Nov 20, 2020

Current Behavior 😯

With today's TypeScript 4.1 release, there's an issue with makeStyles that pops up:

import {createStyles, makeStyles} from '@material-ui/core/styles';
import React from 'react';

export function HeaderBreadcrumbs() {
  const classes = useStyles();
  //              ~~~~~~~~~~~ Expected 1 arguments, but got 0. ts(2554)
  return <div className={classes.outer}>Hello!</div>;
}

const useStyles = makeStyles(theme =>
  createStyles({
    outer: {
      '& .MuiLink-root': {
        ...theme.typography.h6,  // this line is key!
        color: 'inherit',
      },
    },
  }),
);

This passes the type checker with TS 4.0 but not TS 4.1, where you get this error:

Expected 1 arguments, but got 0. ts(2554)
An argument for 'props' was not provided.

This is being tracked on the TypeScript side over here: microsoft/TypeScript#41099

Unclear to me if this is a TS problem or a MUI problem, but it can't hurt to have linked issues on both repos!

Expected Behavior 🤔

No type error (the code works fine at runtime)

Steps to Reproduce 🕹

See code above and in linked issue.

Context 🔦

TypeScript 4.1 update

Your Environment 🌎

Tech Version
Material-UI v4.11.0
React 16.13.1
Browser n/a
TypeScript 4.1.2
etc.
@danvk danvk added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Nov 20, 2020
@kahirokunn
Copy link

I got same problem. 😢

@kahirokunn
Copy link

I tried fix node_modules/@material-ui/core/styles/makeStyles.d.ts for debug.

/**
 * `makeStyles` where the passed `styles` do not depend on props
 */
export default function makeStyles<Theme = DefaultTheme, ClassKey extends string = string>(
  style: Styles<Theme, {}, ClassKey>,
  options?: Omit<WithStylesOptions<Theme>, 'withTheme'>
): (props?: any) => ClassNameMap<ClassKey>;
/**
 * `makeStyles` where the passed `styles` do depend on props
 */
export default function makeStyles<
  Theme = DefaultTheme,
  Props extends {} = {},
  ClassKey extends string = string
>(
  styles: Styles<Theme, Props, ClassKey>,
  options?: Omit<WithStylesOptions<Theme>, 'withTheme'>
): (
- props: Props
+ props?: Props
) => ClassNameMap<ClassKey>;

Type error was solved.

@eps1lon eps1lon added typescript package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5. bug 🐛 Something doesn't work and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Nov 20, 2020
@eps1lon
Copy link
Member

eps1lon commented Nov 20, 2020

Can confirm in our own codebase now. We actually do run tests with TS canaries. But this particular error was hidden by another, less severe error. In the future we should be able to anticipate these regressions better.

@ahwise
Copy link

ahwise commented Nov 20, 2020

Experiencing the same issue; where the culprit for me was ...theme.mixins.toolbar

spreading the object ...theme.mixins.toolbar gives

  @media (min-width:0px) and (orientation: landscape): {minHeight: 48}
  @media (min-width:600px): {minHeight: 64}
  minHeight: 56

so for now ive just appended those lines in place of the spread

    '& @media (min-width:0px) and (orientation: landscape)': { minHeight: 48 },
    '& @media (min-width:600px)': { minHeight: 64 },
     minHeight: 56

@ldrick
Copy link
Contributor

ldrick commented Nov 23, 2020

Can this be fixed in Version 4, please?

@oliviertassinari
Copy link
Member

@ldrick Do you want to open a pull request for it on master?

@oliviertassinari
Copy link
Member

Can this be fixed in Version 4, please?

Done #23692. It's unclear when we will release the changes. The simplest would be to wait around Q1 2021 https://twitter.com/olivtassinari/status/1328346262505205768. So we have enough deprecations to release.

@kahirokunn
Copy link

I want to use of the combination of Typescript 4.1 and Material-UI as soon as possible, do you have any ideas?

@Primajin
Copy link
Contributor

Ahh that's a bummer - I was also hoping for a soon v4 release of this as we will need to work on many upgrades like Typescript 4.1, react 17, ... and preparing for the Material UI v5 release 🎉 . Waiting until Q1 would mean also hold off on all other upgrades and then we would need to do all at once 🙈 - so if there is away (other than sprinkling @ts-ignores over all components) that would be much appreciated 🙏🏻

@oliviertassinari
Copy link
Member

Yeah, in hindsight, I think that we will do something this week. We still need to add support for React v17 and we can release.

@oliviertassinari
Copy link
Member

https://github.com/mui-org/material-ui/releases/tag/v4.11.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5. typescript
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants