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

[Dropdown] Fix imports of types #38296

Merged

Conversation

yash-thakur
Copy link
Contributor

@yash-thakur yash-thakur commented Aug 3, 2023

There was an issue in the latest version related to types which breaks typescript build for everyone.

@yash-thakur yash-thakur changed the title Fix import for types [Dropdown]Fix import for types Aug 3, 2023
@mui-bot
Copy link

mui-bot commented Aug 3, 2023

Netlify deploy preview

https://deploy-preview-38296--material-ui.netlify.app/

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 939c8cb

@yash-thakur yash-thakur marked this pull request as draft August 3, 2023 08:37
@yash-thakur yash-thakur marked this pull request as ready for review August 3, 2023 08:37
@michaldudak
Copy link
Member

Can you describe the issue and provide a reproduction? I'd argue it "breaks typescript build for everyone" as our CI passes without errors.

@michaldudak michaldudak self-requested a review August 3, 2023 09:51
@michaldudak michaldudak changed the title [Dropdown]Fix import for types [Dropdown] Fix import for types Aug 3, 2023
@michaldudak michaldudak added the component: menu This is the name of the generic UI component, not the React module! label Aug 3, 2023
@yash-thakur
Copy link
Contributor Author

Hello @michaldudak

image

This is the issue that our entire team was facing after our CI/CD picked up the latest release.
I reverted the package to the previous version and all the issues were fixed. If you check the screenshot the import { type
should actually have been import type { to work correctly with the typescript compiler.

Let me know if you need any more information.

Thanks!

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 3, 2023
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 3, 2023
@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 3, 2023

The PR looks correct https://www.typescriptlang.org/docs/handbook/modules.html#importing-types, as far as I understand it. We broke compatibility for TypeScript version <4.5, which goes against

Material UI requires a minimum version of TypeScript 3.5. This aims to match the policy of DefinitelyTyped, with the support of the versions of TypeScript that are less than two years old.

https://mui.com/material-ui/getting-started/supported-platforms/#typescript


Material UI requires a minimum version of TypeScript 3.5.

@yash-thakur I'm curious, which MUI open-source project are you using? What's your TypeScript version?

@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work regression A bug, but worse labels Aug 3, 2023
@yash-thakur
Copy link
Contributor Author

@oliviertassinari
I am using typescript version 4.5.5 and using @mui/material in my project.

@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 3, 2023

Strange then, it should work for you https://devblogs.microsoft.com/typescript/announcing-typescript-4-5/#type-on-import-names (the release notes of TS are so great, they cover the problem well each time 😊).

@yash-thakur
Copy link
Contributor Author

Then maybe because my system has overall tsc version as 4.4.4 which is <4.5 that causes the issue.

@michaldudak
Copy link
Member

michaldudak commented Aug 4, 2023

@yash-thakur Thanks for the details. Could you please update the PR to change only the problematic lines and leave the rest as before?

@michaldudak michaldudak changed the title [Dropdown] Fix import for types [Dropdown] Fix imports of types Aug 4, 2023
@michaldudak
Copy link
Member

Thanks for working on this!

@mnajdova mnajdova merged commit 6604389 into mui:master Aug 7, 2023
18 checks passed
richbustos pushed a commit that referenced this pull request Aug 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: menu This is the name of the generic UI component, not the React module! regression A bug, but worse typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants