-
-
Notifications
You must be signed in to change notification settings - Fork 32.5k
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
[typescript-to-proptypes] Support using Omit
on types with conditional properties
#41033
[typescript-to-proptypes] Support using Omit
on types with conditional properties
#41033
Conversation
Netlify deploy previewhttps://deploy-preview-41033--material-ui.netlify.app/ Bundle size report |
ts.TypeFlags.IncludesWildcard
Omit
on types with conditional properties
@@ -55,6 +55,48 @@ function getSymbolDocumentation({ | |||
return comment !== '' ? comment : undefined; | |||
} | |||
|
|||
function getType({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just moved this code without any change
@@ -206,6 +248,38 @@ function checkType({ | |||
return createLiteralType({ jsDoc, value: 'null' }); | |||
} | |||
|
|||
if (type.flags & ts.TypeFlags.IndexedAccess) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that, for conditional types inside an Omit
, TS does not resolve its value, it just says "it's looks like A['key']
".
My ugly solution is to retrieve its resolved parent with objectType
, and if this parent looks like type A<T> = T extends B ? C : D
, then I resolve both sides of the ternary and for each I get the property value and create an union.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO, it seems only partially hacky.
We have a known case that we handle conditionally, it does not produce any side effects in the Core
repo and none in the X
repo—sounds like a solution to me. 🤔 🙈 💡
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sens for the generation of proptypes 👍
I assume TS only keep the index pointing to it in order to easily manage the case where the condition get defined. Which we don't care for proptypes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice and very needed improvement! 👍 🚀
@@ -206,6 +248,38 @@ function checkType({ | |||
return createLiteralType({ jsDoc, value: 'null' }); | |||
} | |||
|
|||
if (type.flags & ts.TypeFlags.IndexedAccess) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO, it seems only partially hacky.
We have a known case that we handle conditionally, it does not produce any side effects in the Core
repo and none in the X
repo—sounds like a solution to me. 🤔 🙈 💡
@@ -206,6 +248,38 @@ function checkType({ | |||
return createLiteralType({ jsDoc, value: 'null' }); | |||
} | |||
|
|||
if (type.flags & ts.TypeFlags.IndexedAccess) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sens for the generation of proptypes 👍
I assume TS only keep the index pointing to it in order to easily manage the case where the condition get defined. Which we don't care for proptypes
I don't know why TS does not work correctly with this type combination and this fix is super ugly.
But it works 😬