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] Simplify some of the conditional types #18128

Merged
merged 2 commits into from
Nov 1, 2019

Conversation

amcasey
Copy link
Contributor

@amcasey amcasey commented Nov 1, 2019

These tweaks seem to reduce the number of types generated by the TS compiler, but not by enough to substantially impact performance. Mostly, they seem to simplify the code.

@mui-pr-bot
Copy link

mui-pr-bot commented Nov 1, 2019

No bundle size changes comparing 6610686...9b9b6d6

Generated by 🚫 dangerJS against 9b9b6d6

@eps1lon eps1lon added package: types Specific to @mui/types performance labels Nov 1, 2019
@eps1lon
Copy link
Member

eps1lon commented Nov 1, 2019

I'll trust you regarding performance impact. I'm a fan of the simpler code though. Makes it more obvious what the type does without looking at the name 👍

Anything more you want to add before leaving Draft state?

@amcasey amcasey marked this pull request as ready for review November 1, 2019 17:18
@amcasey
Copy link
Contributor Author

amcasey commented Nov 1, 2019

@eps1lon All good, just didn't want to bother anyone while the validation was running.

@eps1lon eps1lon changed the title Simplify some of the conditional types in material-ui-types [types] Simplify some of the conditional types Nov 1, 2019
@eps1lon eps1lon merged commit 9d0caa5 into mui:master Nov 1, 2019
@eps1lon
Copy link
Member

eps1lon commented Nov 1, 2019

@amcasey Much appreciated, thanks!

amcasey added a commit to amcasey/material-ui that referenced this pull request Nov 2, 2019
This reverts commit 9d0caa5.

The new version of `IsEmptyInterface` accepts objects with all optional
properties and `And` and `Or` actually have negative perf impact on
their own.
@amcasey amcasey deleted the SimplifyUtilityTypes branch November 2, 2019 00:38
@amcasey
Copy link
Contributor Author

amcasey commented Dec 11, 2019

@eps1lon Is there someone I can work with to better understand the intentions behind the material-ui typings? We're still getting perf complaints and, while there's definitely room for the compiler to improve, some of the types surprised us with their complexity - maybe they can be simplified. Thanks!

@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 11, 2019

@amcasey Maybe in #18789?

@eps1lon
Copy link
Member

eps1lon commented Dec 11, 2019

@amcasey That would be me. Best open a new issue and ask away what types in particular you need clarification with.

@amcasey
Copy link
Contributor Author

amcasey commented Dec 11, 2019

@eps1lon Thanks! I think I worked out most of the answers I was looking for through trial and error, but I'll connect with you in the new year about what combination of TS and material-ui changes would best address the customer request.

In the meantime, do you want an issue tracking the fact that this is causing pain for users?

@eps1lon
Copy link
Member

eps1lon commented Dec 12, 2019

Yeah. I'll reference some more issues that I think are related and then we can tackle this in the new year.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: types Specific to @mui/types performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants