[styles] Add support for TypeScript 4.1 #23692
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Details of bundle changes.Comparing: 2f8b294...45dba02 Details of page changes
|
oliviertassinari
changed the title
oliviertassinari
added
package: styles
|
@ldrick It's a great first pull request on Material-UI 👌🏻. Thank you for working on it! |
2 tasks
amankkg
reviewed
Nov 26, 2020
| export default function makeStyles< | ||
| Theme = DefaultTheme, | ||
| Props extends {} = {}, | ||
| Props extends object = {}, |
There was a problem hiding this comment.
This change is a breaking one for me
// Type 'boolean' does not satisfy the constraint 'object'.ts(2344)
// ___________________________________🔻
const useStyles = makeStyles<Theme, boolean>(() =>
createStyles({
root: {
display: (show) => (show ? 'block' : 'none'),
}
})
}There was a problem hiding this comment.
Probably, I was using it not as it was designed but still, FYI if it was unintentional
There was a problem hiding this comment.
Related to #23706 (comment)
There was a problem hiding this comment.
A bit late to the party, but I wanted to say that we also found this (after a day and a half of searching) to be a breaking change. Based on the docs here, I think lots of people may have implemented a function instead of an object... I'm also not sure I've fully wrapped my head around why it broke us, but that's sort of beside the point. Just wanted to request more caution around things like this being in a patch release even if you could argue people were using it wrong as a justification for why it's "not a breaking change".
I would argue that if it was ever possible to use it wrong, and then you remove that possibility, that you've just made a breaking change, even if that was never the intended usage. /shrug
There was a problem hiding this comment.
Just wanted to request more caution around things like this being in a patch release even if you could argue people were using it wrong as a justification for why it's "not a breaking change".
Any type change is potentially backwards incompatible and can therefore be considered breaking. In the DefinitelyTyped community, as well as recommended by TypeScript core members, we don't bump SemVer major changes if patch types or add new features. Otherwise we couldn't apply any change without constantly bumping the major.
amankkg
reviewed
Nov 26, 2020
| ): (props: Props) => ClassNameMap<ClassKey>; | ||
| ): keyof Props extends never // `makeStyles` where the passed `styles` do not depend on props | ||
| ? (props?: any) => ClassNameMap<ClassKey> // `makeStyles` where the passed `styles` do depend on props | ||
| : (props: Props) => ClassNameMap<ClassKey>; |
There was a problem hiding this comment.
Those comments should be on lines 14 & 15 I guess 😅
amankkg
reviewed
Nov 26, 2020
| ): (props: Props) => ClassNameMap<ClassKey>; | ||
| ): keyof Props extends never // `makeStyles` where the passed `styles` do not depend on props | ||
| ? (props?: any) => ClassNameMap<ClassKey> // `makeStyles` where the passed `styles` do depend on props | ||
| : (props: Props) => ClassNameMap<ClassKey>; |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds cherry picks support for TypeScript 4.1 #23633 fixing #23627 in version 4.
Thanks to @eps1lon for creating the original PR.