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

[core] Use type imports for theme augmentation #14383

Closed
wants to merge 4 commits into from

Conversation

Janpot
Copy link
Member

@Janpot Janpot commented Aug 29, 2024

These are importing from a .d.ts file, meaning, here is no code to import from, this can confuse build tools such as in #14234. Best to either use .ts files, or use a type import.

Question: Any reason to not just use .ts files? These manual type declarations, while supported, feel a bit like an anti-pattern to me.

@Janpot Janpot added the core Infrastructure work going on behind the scenes label Aug 29, 2024
@mui-bot
Copy link

mui-bot commented Aug 29, 2024

Deploy preview: https://deploy-preview-14383--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against c0ddee2

@Janpot Janpot marked this pull request as ready for review August 29, 2024 10:22
@alexfauquette
Copy link
Member

Question: Any reason to not just use .ts files? These manual type declarations, while supported, feel a bit like an anti-pattern to me.

@flaviendelangle will have more info, but I think it's just a copy/past from the @mui/lab files that got copy/pasted to other components (tree view, and charts)

https://github.com/mui/material-ui/blob/master/packages/mui-lab/src/themeAugmentation/components.d.ts

This would explain why the data-grid uses .ts file

@flaviendelangle
Copy link
Member

@flaviendelangle will have more info, but I think it's just a copy/past from the @mui/lab files that got copy/pasted to other components (tree view, and charts)

It is 😆
If the grid is using a better format and it works for years, then I see no reason not to migrate to their format 👍

Copy link
Member

@LukasTy LukasTy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the change. 👍
I fear that renaming the files could be a niche breaking change.
We could align the approach and rename files on v8.

Or we can refactor the PR to rename files and merge it to next branch when we start working on it. 🤔

@Janpot
Copy link
Member Author

Janpot commented Sep 17, 2024

I fear that renaming the files could be a niche breaking change.

I kind of expect the type generation to have the exact same output. Will test.

@Janpot
Copy link
Member Author

Janpot commented Sep 18, 2024

Looks like the difference in output is:

  • this PR:
    output declaration files re-export types with export type, instead of just export. No js modules are generated
  • renaming the files:
    output declaration files re-export with just export, just like we currently do. But it also generates empty js modules

I don't see how either of these could lead to a breaking change for the user. The .ts method already works for the data grid anyway.

@Janpot
Copy link
Member Author

Janpot commented Sep 18, 2024

It looks like since I made this PR, they have been updated already on master. Closing this for now, X repo satisfies the needs for #14234 now. Up to the team if they want to align further.

@Janpot Janpot closed this Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants