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

makeStyles typescript issue when using function type attribute on 4.11.1 #23706

Closed
atnpcg opened this issue Nov 24, 2020 · 13 comments
Closed

makeStyles typescript issue when using function type attribute on 4.11.1 #23706

atnpcg opened this issue Nov 24, 2020 · 13 comments
Labels
bug 🐛 Something doesn't work typescript
Milestone

Comments

@atnpcg
Copy link

atnpcg commented Nov 24, 2020

after updating to material-ui-core 4.11.1
I have a typescript error when using makeStyles using attributes in a function type

  • [x ] The issue is present in the latest release.
  • [x ] I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

I get a Typescript error saying props are not compatible with props

Expected Behavior 🤔

should not have any errors

I couldn't' configure the playground with recent versions. but you can copy paste the code to version 4.11.1 and you will see the typescript error.
on version 4.11.0 is not happening

https://www.typescriptlang.org/play?#code/JYWwDg9gTgLgBAKjgQwM5wEoFNkGN4BmUEIcA5FDvmQNwBQokscA3nXHAPSdwwAWWOLhKQAdllEx0ATwgBXOHNRYAJnQC+cIiXIABEMhhYowZABsAtHOCdhlMg3DR4LOAYDWWAMoxpZrOia2qRk+obGppbWttBYnKi+-qi0dKm+YIIAKgEwAArEYOgAvKzsHHBgBQCMAFxwCSaiAOb05RUFAEx1DcDNreWVEGAAzAD83TCNLRr0dMKiCYrKPn4BcCUe3okBABQ7AJTrAHxwO2xtxBAwdTuDhXW5wLjuADzZCflDqAA05HdVZDgAB8-p0yEdDkUTuc2uVKDA5FBRKVYai4CpgKgwGZkNI6mQCP4AB5kb5lVHqfocdRk8m8HI3O6oB5PV7vPIFH6gobDcGQ6F0i5YBFIlFotoYrE4vHkQlYElk8WUuk0jT7fb0AhyUT4YAQZEAESwIAgBzF3DgAE15FAhBAVFgNEA

Tech Version
Material-UI v4.11.1
React 16.13.1
Browser chrome
TypeScript 3.8.3
etc.
@atnpcg atnpcg added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Nov 24, 2020
@atnpcg atnpcg changed the title makeStyles typescript issue when using Pick on 4.11.1 makeStyles typescript issue when using function type attribute on 4.11.1 Nov 24, 2020
@eps1lon
Copy link
Member

eps1lon commented Nov 25, 2020

Support for TypeScript 4.1 shouldn't have been released since we decided to not release features for 4.x anymore. Seems like this feature came with new bugs which is exactly the reason why you usually don't release features for LTS releases. Sorry about that.

I recommend you roll back to 4.11.0.

@eps1lon eps1lon added bug 🐛 Something doesn't work typescript and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Nov 25, 2020
@eps1lon eps1lon added this to the v5 milestone Nov 25, 2020
@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 25, 2020

@eps1lon What do you think about this alternative read?

  • Few developers uses v5 yet.
  • What we think we fix in v5 isn't stress-tested, so we don't know for sure.
  • We extend the maintenance policy in v4 to include important bug fixes and more importantly to stay up with the third-party dependencies we can't control (React, TypeScript, Browser, JSS, npm, etc). We still don't release new features, and stays in the spirit of the initial policy. The motivation is to not force people on older versions which could be a deterrent against using Material-UI. It's one thing to freeze the development of Material-UI when on v4, it's another to have to stay on older third-party dependencies when using it.
  • We leverage v4 to stress-test the important fixes we land in v5 be releasing them on v4 too. This gives us a shorter feedback loop. So we don't have to wait for 6 months.

@oliviertassinari
Copy link
Member

@atnpcg Do you have a reproduction on Codesandbox? It would help :)

@eps1lon
Copy link
Member

eps1lon commented Nov 25, 2020

If you find someone to do it, sure. I don't have time for it.

@atnpcg
Copy link
Author

atnpcg commented Nov 25, 2020

hi @oliviertassinari here is a sandbox with the new version:
https://codesandbox.io/s/loving-bash-mc7cu?file=/src/App.tsx
and here is the same code with 4.11.0
https://codesandbox.io/s/crimson-bird-4qu5o?file=/src/App.tsx

@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 25, 2020

@atnpcg Thanks. Are you should about the previous TypeScript logic?

I was expecting to see:

const useStyles = makeStyles(() => ({
  root: (props: TestProps) => {
    return {
      display: "flex"
    };
  },
  test: (props: TestProps) => {
    return {
      display: "flex"
    };
  }
}));

https://codesandbox.io/s/dark-microservice-kpivc?file=/src/App.tsx:243-443

Why use Pick?

@atnpcg
Copy link
Author

atnpcg commented Nov 25, 2020

@oliviertassinari, of course it can be fixed by changing the code, the code I provided is jut a minimal example, the code I detected the issue was in a more complex project with more complex props.
but regardless there should not a breaking change in a patch version

@oliviertassinari
Copy link
Member

regardless there should not a breaking change in a patch version

If wasn't meant to be a breaking change. I guess want to comes to TypeScript, there isn't any semver? My question would be more about. Is your example correct? I don't really understand the motivation for using different types for the props. Could it actually surface a behavior that was previously wrong?

@atnpcg
Copy link
Author

atnpcg commented Nov 25, 2020

@oliviertassinari , that's a good question!, I think we are using pick all over our project, thinking that we don't need all the props of a given type for each function.
but I haven't check what's the expected behavior for makeStyles functions, (if any function can receive different props, or it has to be the same)
Thanks

@maccuaa
Copy link

maccuaa commented Nov 25, 2020

I think this happens with any function in makeStyles. After upgrading to 4.11.1 my component would no longer compile which is not doing anything out of the ordinary:

const useStyles = makeStyles({
  wrapper: {
    display: "flex",
    alignItems: "center",
  },
  icon: {
    marginRight: 16,
    minWidth: (size: number) => size,
    width: (size: number) => size,
    height: (size: number) => size,
  },
});

The TS error is:

Type '(size: number) => number' is not assignable to type 'string | number | PropsFunc<object, string | number | undefined> | undefined'.
  Type '(size: number) => number' is not assignable to type 'PropsFunc<object, string | number | undefined>'.
    Types of parameters 'size' and 'props' are incompatible.
      Type 'object' is not assignable to type 'number'.

@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 25, 2020

@maccuaa The argument of makeStyles() is designed to be used as an object.

@maccuaa
Copy link

maccuaa commented Nov 25, 2020

Ah ok. Changing it to this fixed the issue:

const useStyles = makeStyles({
  icon: {
    marginRight: 16,
    minWidth: (props: Props) => props.size,
    width: (props: Props) => props.size,
    height: (props: Props) => props.size,
  },
});

But to @atnpcg's point, my first snippet worked fine in 4.11.0 but broke in 4.11.1 which is not something you expect to happen in a patch release. But I guess the original implementation was wrong so the fix in 4.11.1 breaks everyone who's been using it wrong this entire time 😛

@oliviertassinari
Copy link
Member

Closing as the change while breaking was surfacing incorrect usages, at least, not how we designed the API to be used. Thanks for raising!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work typescript
Projects
None yet
Development

No branches or pull requests

4 participants