Refine style types #1554
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,7 +16,7 @@ const expectedCustomTheme: MyTheme = {color: 'red'} | |
|
|
||
| /* -------------------- THEME ARGUMENT -------------------- */ | ||
| // Regular, static styles work fine | ||
| const themeArg1 = createUseStyles((theme) => ({ | ||
| someClassName: '', | ||
| anotherClassName: { | ||
| fontWeight: 'bold' | ||
|
|
@@ -26,20 +26,20 @@ const themeArg1ClassesPass = themeArg1() | |
|
|
||
| // Theme type assumed to be the default | ||
| // Nested theme declaration banned | ||
| const themeArg2 = createUseStyles((parentTheme) => ({ | ||
| themeNotAllowed: ({theme}: {theme: MyTheme}) => ({ | ||
| color: theme.color | ||
| }) | ||
| })) | ||
| // @ts-expect-error | ||
| const themeArg2ClassesFail = themeArg2({theme: {}}) | ||
| // @ts-expect-error | ||
| const themeArg2ClassesFail2 = themeArg2({theme: expectedCustomTheme}) | ||
| // @ts-expect-error | ||
| const themeArg2ClassesFail2 = themeArg2({theme: expectedDefaultTheme}) | ||
|
|
||
| // Props declaration is allowed | ||
| const themeArg3 = createUseStyles<string, MyProps>((theme) => ({ | ||
| onlyPropsAllowed: ({...props}) => ({ | ||
| fontWeight: 'bold' | ||
| }) | ||
|
|
@@ -52,16 +52,15 @@ const themeArg3ClassesPass = themeArg3(expectedCustomProps) | |
| const themeArg3ClassesPass2 = themeArg3({...expectedCustomProps, theme: expectedDefaultTheme}) | ||
|
|
||
| // Nested props declaration banned | ||
| const themeArg4 = createUseStyles<string, MyProps>((theme) => ({ | ||
| onlyPropsAllowed: ({...props}) => ({ | ||
| fontWeight: 'bold', | ||
| propsNotAllowed: ({...innerProps}) => '' | ||
| }) | ||
| })) | ||
|
|
||
| // Supplied theme type is acknowledged | ||
| const themeArg5 = createUseStyles<string, unknown, MyTheme>((theme) => ({})) | ||
| // @ts-expect-error | ||
| const themeArg5ClassesFail = themeArg5({theme: {}}) | ||
| // @ts-expect-error | ||
|
|
@@ -81,7 +80,7 @@ const themeArg6ClassesFail2 = themeArg6({theme: expectedDefaultTheme}) | |
| const themeArg6ClassesPass = themeArg6({theme: expectedCustomTheme}) | ||
|
|
||
| // Props can be determined implicitly | ||
| const themeArg7 = createUseStyles((theme) => ({ | ||
| checkbox: ({property}: MyProps) => ({ | ||
| borderColor: property | ||
| }) | ||
|
|
@@ -172,53 +171,14 @@ const noThemeArg4ClassesPass2 = noThemeArg4({...expectedCustomProps, theme: expe | |
| const noThemeArg5 = createUseStyles<string, MyProps, MyTheme>({ | ||
| singleNest: { | ||
| fontWeight: 'bold', | ||
| // @ts-expect-error | ||
| nestOne: ({property, theme}: MyProps & {theme: MyTheme}) => ({ | ||
| color: 'red', | ||
| display: () => 'block' | ||
| }) | ||
| } | ||
| }) | ||
|
|
||
|
There was a problem hiding this comment. I don't clearly understand what this is testing, because nesting is not generally disallowed, its just only working with nested syntax with & |
||
|
There was a problem hiding this comment. why would one need double nest and triple nest test, is there code that is explicitly working in one of them and not in another? There was a problem hiding this comment. I know merging of some commits happened earlier. Not sure if this PR holds any of those. My comments on those kinds of tests are probably misleading. What it should say is probably something like
In terms of function declarations, no, I'm not aware of any existing code that tries to nest function declarations. But I thought that I read on the docs somewhere (or in a console warning) that nested functions were not supported. This test was just to ensure that the types would also error out if someone tried to create a nested function (before the runtime bites them). I did "double" and "triple" failure tests for function declarations just to make sure the type was appropriately defined. This was for two main reasons:
Theoretically speaking, a person could nest as much as they want. But testing every Nth scenario would be impractical. I stopped at 2nd and 3rd level nesting to verify that our recursive types were properly defined. There was a problem hiding this comment. This probably also answers #1554 (comment) There was a problem hiding this comment. The confusing part was triple test, if we just forbid double nest its already solves the problem There was a problem hiding this comment. Okay. If you think the double nest is sufficient, I think we can remove all the triple-nest tests. Makes the file shorter 😅 There was a problem hiding this comment. I mean if double nesting is already prevented, x nesting is also prevented. It was just confusing to see 3x level of nesting because I start thinking why would you need to test a 3rd level, whats special about it |
||
| // Props can be determined implicitly | ||
| const noThemeArg8 = createUseStyles({ | ||
| checkbox: ({property}: MyProps) => ({ | ||
|
|
||
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.
This was supposed to be Data all along, the fact it can be Props is up to user, it could be anything