mui / material-ui

Material UI: Comprehensive React component library that implements Google's Material Design. Free forever.
https://mui.com/material-ui/
MIT License
93.91k stars 32.27k forks source link

[mui/system] theme - typescript module augmentation #43855

Closed yonatan0 closed 4 weeks ago

yonatan0 commented 1 month ago

Steps to reproduce

Link to live example: (required)

https://codesandbox.io/p/devbox/6tvy25

Steps:

  1. created theme.ts and theme.d.ts
  2. adjusted @mui/system module

Current behavior

getting error from my Main component 'theme.typography' is of type 'unknown'.ts(18046)

Expected behavior

typography to be regognized

Context

creating my own theme using only @mui/system

Your environment

npx @mui/envinfo ``` System: OS: Linux 5.15 Ubuntu 22.04.3 LTS 22.04.3 LTS (Jammy Jellyfish) Binaries: Node: 20.17.0 - ~/.nvm/versions/node/v20.17.0/bin/node npm: 10.8.2 - ~/.nvm/versions/node/v20.17.0/bin/npm pnpm: Not Found Browsers: Chrome npmPackages: @emotion/react: ^11.13.3 => 11.13.3 @emotion/styled: ^11.13.0 => 11.13.0 @mui/material-nextjs: ^6.1.0 => 6.1.0 @mui/private-theming: 6.1.0 @mui/styled-engine: 6.1.0 @mui/system: ^6.1.0 => 6.1.0 @mui/types: 7.2.16 @mui/utils: 6.1.0 @types/react: ^18 => 18.3.7 react: ^18 => 18.3.1 react-dom: ^18 => 18.3.1 typescript: ^5 => 5.6.2 ```

Search keywords: typescript module augmentation mui/system

siriwatknp commented 1 month ago

@yonatan0 Can you check the permission of the CodeSandbox? I could not open it.

yonatan0 commented 1 month ago

@yonatan0 Can you check the permission of the CodeSandbox? I could not open it.

My bad, please try now 🙏

mnajdova commented 1 month ago

The problem is that with module augmentation you can only extend the interface, but you can't change its defined properties' types. Maybe instead of defining these props as unknown we can omit them. I am not sure what would be the best approach in this case. @Janpot do you have any suggestion?

Janpot commented 1 month ago

Would it make sense to declare an empty Typography interface in @mui/system and use it as the type of the typography property? One could then augment this interface instead.

mnajdova commented 1 month ago

That makes sense too, we could do it for the other properties that have the unknown type. @yonatan0 would you like to create a PR for this? All changes should be contained in this file: https://github.com/mui/material-ui/blob/master/packages/mui-system/src/createTheme/createTheme.d.ts

yonatan0 commented 1 month ago

That makes sense too, we could do it for the other properties that have the unknown type. @yonatan0 would you like to create a PR for this? All changes should be contained in this file: https://github.com/mui/material-ui/blob/master/packages/mui-system/src/createTheme/createTheme.d.ts

I've created PR. Hopefully i understood you correctly :)

mnajdova commented 4 weeks ago

This was fixed in https://github.com/mui/material-ui/pull/43873. I am closing the issue. Thanks for the fix @yonatan0. Note you can use the "Fixes https://github.com/mui/material-ui/issues/43855" for automatically closing the issue.

github-actions[bot] commented 4 weeks ago

This issue has been closed. If you have a similar problem but not exactly the same, please open a new issue. Now, if you have additional information related to this issue or things that could help future readers, feel free to leave a comment.

[!NOTE] @yonatan0 How did we do? Your experience with our support team matters to us. If you have a moment, please share your thoughts in this short Support Satisfaction survey.