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

[types] Moved types from OverridableComponent.d.ts to @material-ui/types #23265

Closed

Conversation

mnajdova
Copy link
Member

Another PR that prepares unnecessary changes for introducing the @material-ui/unstyled package. It moves the typings from @material-ui/core/OverridableComponent to @material-ui/types so that those can be reused in the new package. All previous typings from @material-ui/core/OverridableComponent are now just re-exported from @material-ui/types

@mui-pr-bot
Copy link

mui-pr-bot commented Oct 26, 2020

No bundle size changes

Generated by 🚫 dangerJS against 41d7b2b

Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

We need to fork at some point in the chain of used types started from OverrideProps since that type is coupled with @material-ui/styles. StyledComponentProps or ClassNameMap should not be part of @material-ui/types.

@mnajdova
Copy link
Member Author

We need to fork at some point in the chain of used types started from OverrideProps since that type is coupled with @material-ui/styles. StyledComponentProps or ClassNameMap should not be part of @material-ui/types.

CommonProps is the types that uses the StyledComponentProps, the ClassNameMap is added because it is used in StyledComponentProps. Not sure how we can fork it as this type is required in the OverridableComponent eventually. Do you have some suggestion?

@mnajdova
Copy link
Member Author

Or we could exclude the StyledComponentsProps, as we are anyway not using the classes API anymore. And we can have a different typing for the CommonProps in core for now for the components that still use this API

@mnajdova mnajdova requested a review from eps1lon October 26, 2020 11:09
@mnajdova
Copy link
Member Author

@eps1lon can you check now?

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

From what I understand, the purpose of @material-ui/types is to host type utilities that are shared between different packages. Two questions:

  1. Why create a distinction between JavaScript logic and TypeScript only logic. Would it make sense to merge @material-ui/types inside @material-ui/utils? (keeping them both for private usage)
  2. It seems that these types don't need to be shared. Would it be simpler to host them in @material-ui/unstyled and have core import them from unstyled?

Comment on lines +135 to +144
/**
* @deprecated Not used in this library.
*/
export type Simplify<T> = T extends any ? { [K in keyof T]: T[K] } : never;

/**
* @deprecated Not used in this library.
*/
// tslint:disable-next-line: deprecation
export type SimplifiedPropsOf<C extends React.ElementType> = Simplify<React.ComponentProps<C>>;
Copy link
Member

Choose a reason for hiding this comment

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

Should we remove it?

Suggested change
/**
* @deprecated Not used in this library.
*/
export type Simplify<T> = T extends any ? { [K in keyof T]: T[K] } : never;
/**
* @deprecated Not used in this library.
*/
// tslint:disable-next-line: deprecation
export type SimplifiedPropsOf<C extends React.ElementType> = Simplify<React.ComponentProps<C>>;

Copy link
Member

Choose a reason for hiding this comment

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

Yes, please!

@oliviertassinari oliviertassinari added the core Infrastructure work going on behind the scenes label Oct 26, 2020
@eps1lon
Copy link
Member

eps1lon commented Oct 26, 2020

Why create a distinction between JavaScript logic and TypeScript only logic.

One has zero runtime cost the other doesn't. We don't create the distinction but reflect it. But I'm fine with unifiying them so that we have less stuff to maintain. Though we should really just move @material-ui/utils to @material-ui/internal to make it clear that we don't want to maintain a (perceived) public contract.

Would it be simpler to host them in @material-ui/unstyled and have core import them from unstyled?

That sounds more reasonable.

@mnajdova
Copy link
Member Author

So is the best way to start moving to the unstyled package everything we find required along the way and avoid splitting utils and types to different packages?

If that's what we want I will close this and #23264 and start with just creating the unstyled package and move everything required there...

@eps1lon @oliviertassinari do we agree with this?

@oliviertassinari
Copy link
Member

@mnajdova Sound fair. I'm not sure that we need to give up on the two existing pull requests.

@mnajdova
Copy link
Member Author

@mnajdova Sound fair. I'm not sure that we need to give up on the two existing pull requests.

Let's open that one and we'll see if it can be broken down to more PRs

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.

4 participants