Refine style types #1554
Refine style types #1554
Conversation
|
This is a work in progress, before we can merge it I would like to turn all tests into ts files so that we have everything covered by the existing tests. But I would love a pre-review already at this stage, because this is going to be a big PR. |
| }) | ||
| } | ||
| }) | ||
|
|
||
| // Nested declarations are banned (double nest test) | ||
| const noThemeArg6 = createUseStyles<string, MyProps, MyTheme>({ |
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 &
| } | ||
| }) | ||
|
|
||
| // Nested declarations are banned (triple nest test) |
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
Nested function declarations are banned
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:
- If a person was doing regular nesting that didn't involve functions, they should be able to define a stand-alone, un-nested function wherever they please.
- If a person tried to be sneaky and say, "Maybe I can nest function declarations if I start at a deeper point in the nest", this test ensures that they'd be prevented from doing so.
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
| export type Styles< | ||
| Name extends string | number | symbol = string, | ||
| Props = unknown, | ||
| Data = unknown, |
There was a problem hiding this comment.
This was supposed to be Data all along, the fact it can be Props is up to user, it could be anything
|
I haven't looked at the [currently last] commit 04e94fb yet, but the preceding commits seem fine. Regarding 04e94fb, it might be helpful to split out the prettier changes into a preceding (or succeeding) |
| | Func<Data, Theme, JssStyle<undefined, undefined> | string | null | undefined> | ||
| | MinimalObservable<JssStyle | string | null | undefined> | ||
| > | ||
| | Record<`@keyframes ${string}`, Record<'from' | 'to' | `${number}%`, JssStyle<Data, Theme>>> |
Trying to make the types more accurate and ideally improve error messages and tsc performance in the end.