fix: resovle createUseStyle generics order

[hosseinmd]

Corresponding issue (if exists):

https://github.com/cssinjs/jss/pull/1460

Currently, for add props types we have to add classes name first.

What would you like to add/fix?

I changed order of generic types of createUseStyle Now we don't have to add classes name, because classes names recognizing from inserted object.

Todo

• [ ] Add test that verifies the modified behavior
• [ ] Add documentation if it changes public API

Changelog

Please summarize the changes in a way that makes sense inside the changelog. Feel free to add migration tips or examples if necessary.

[ITenthusiasm]

@kof Please see the entire discussion that happened from this comment onward in the linked PR.

If you decide you want to go with these changes, that's fine. But from what I understand, I strongly recommend leaving the code as is.

[hosseinmd]

That hard to define props, we don't have to define classes names.

[kof]

Lets discuss this here, because the linked discussion is on a closed PR and this one seems like a good place to figure it out:

Do I understand correctly that you see different compiler errors from the same types? If yes what can be the reason, different TS versions?

[ITenthusiasm]

The complaint doesn't seem to be about getting compiler errors for the same types. The complaint seems to be that hosseinmd doesn't like having to specify RuleNames first in order for him to specify Props.

[kof]

@hosseinmd do you say the RuleNames type is inferred correctly for you and allows you to get the errors when using classes without manually specifying the RuleNames?

[kof]

I am actually wondering if it's possible to not have to specify RuleNames, because technically they are already there in the object and TS could use those keys and map them to classes

[ITenthusiasm]

I am actually wondering if it's possible to not have to specify RuleNames, because technically they are already there in the object and TS could use those keys and map them to classes

@kof Unfortunately, that's not possible. I found this out the hard way while working on #1460, and k-yle seems to understand this problem as well (see #1453). I'll try to go through the logic of why... but it's a lot of logic. Note that I'm going to try to condense this logic, and there is much more that can be said about why we're stuck in the situation of needing to define RuleNames.

We Need Record<Keys, Values>

In order for users to get the correct types on their objects (properly typed data functions, CSS properties, etc.), we have to type the values of the object the user defines. But in order for us to type the values of the object, we also have to type the properties of the object themselves. This necessitates something like Record<Keys, Values> or { [Key in string]: Value }.

This issue is why React-JSS has the Styles type in the first place. It's basically a simplified version of the necessary Record<Keys, Values> type that users need to get intellisense on their style objects.

We Need a Type for Keys in Record<Keys, Values>

But the need to define keys in the first place is the problem. Let's say we have some general object value type, like JssStyles. If we define an object as Record<string, JssStyles>, then TS will never provide intellisense on the object properties. We explicitly told TypeScript, "You only need to enforce that the properties are strings." So this basically tells TypeScript, "You can use any string you want as a property".

Generic Types Help Us Get around Defining a Type for Keys

In React-JSS styles, we get around this by using generic types. This is because TypeScript will try to infer the generic types if we don't provide any type arguments. So if we define a new type Styles<Names> = Record<Names, JssStyles>, we can "force" TypeScript to infer the object properties. So for instance, doing this:

function someFunction<Name extends string | number | symbol>(input: Styles<Name>) {
  // ...
}

const myStyles = {
  ruleName: { /* ... */ },
};

const output = someFunction(myStyles);

Will force TypeScript to infer that input must be of type Record<"ruleName", JssStyles> when called with myStyles because we didn't provide any generic type arguments. This is what many React-JSS users have been relying on. But here lies the problem.

TypeScript does not Infer any Types When You Supply Generic Type Arguments

This is the thing that trips everyone up. And this is the key issue that k-yle, myself, and other developers have come to recognize. We're relying on the fact that TypeScript infers RuleNames. But TypeScript stops inferring any types once the user supplies even a single generic type argument. Because this is a very niche, fairly uncommon scenario, a lot of people can't wrap their heads around this problem until they run into it themselves. But it's true.

Microsoft has tons of users complaining about this in confusion. You can see it in my microsoft/TypeScript#43119 and in someone else's microsoft/TypeScript#39008. And Ryan (member of Microsoft) makes it very clear that once you supply any generic type arguments, the remaining generic type parameters are forced to their defaults.

The Conclusion

All of this means that if we use a system like <Props = unknown, Theme = DefaultTheme, Names extends string = string>, users will try to specify Props by doing const useStyles = createUseStyles<Props> only. But then they will inevitably lose type information on Names. More accurately, Names will be not be inferred as LIST_OF_STRINGS, but will instead be defaulted to string.

We could try to remove Names so that TypeScript can't default us to Record<string, JssStyles>, but then TypeScript would have no way to infer the Keys because we've removed that information. As Ryan said, we're in this situation because of a poor design in TypeScript, but that's just what we're stuck with.

This is why <Names, Props, Theme> is the only possible order. Users will always have to specify Names once they supply any generic type arguments. So it's better to make them do that up front instead of forcing them to reach over other irrelevant types (eg. Theme) to supply info for Names. More importantly, once microsoft/TypeScript#26349 gets merged, users will be able to do const useStyles = createUseStyles<_, Props>, which is much less painful.

In the meantime, I've tried to add docs to show people how to force TypeScript to infer all types. This is why I recommended the function argument for createUseStyles. Speaking strictly from a standpoint of TypeScript, it is the clearest, least confusing approach, though the object variant is still somewhat manageable.

[ITenthusiasm]

I should note that once microsoft/TypeScript#26349 gets merged, I'm hoping we can get away with something like this:

declare function createUseStyles<Props = unknown, Theme = DefaultTheme>(
  styles: Styles<_, Props, Theme> | ((theme: Theme) => Styles<_, Props, undefined>),
  options?: CreateUseStylesOptions<Theme>
): (data?: Props & {theme?: Theme}) => ClassesForStyles<typeof styles>

But that's mildly hackish and it's not readily apparent if that would be "legal" once the new Microsoft changes get merged. Additionally, if we applied this change, users would never be able to specify strict rules on RuleNames. So even if this becomes possible in the future, we'll be forced with having to decide between:

1. Enabling users to get intellisense for classes in the easiest possible way or
2. Leaving RuleNames as something users will have to force TypeScript to infer through the _ sigil. (Thus, the user would still have full control over RuleNames type enforcements.)

~Regardless of the outcome for createUseStyles, any people using the Styles type for any reason would still have to provide RuleNames or the _ sigil explicitly.~

Correction: Using the _ sigil could potentially simplify the Styles type as well -- hopefully.

[k-yle]

Adding to @ITenthusiasm's explanation: here's an example in the typescript playground that shows that the order of the generics makes no difference

[kof]

To understand the problem I am trying to reproduce it on a simple playground, here is what I got so far, but I am guessing it's not representing the actual problem. @ITenthusiasm what do I need to add there to make it show the problem? this will be maybe a good example to explain for future readers https://www.typescriptlang.org/play?jsx=0&ts=4.2.3#code/MYewdgzgLgBFIGEA2BDCECmEYF4YB4BlKATySwD4AKaMrALhgG8YBtAJQFdyA5FAWwwwAlmBgBrDCRAAzGMToQAuo2gAnUQHMYAXwCUuCjFrkIAbgCwAKGuhIsGWpD8FpgCqJU6LLgKvKNKSmjP4QBjhGTNYwMHbQsV6YEIwoYCS+TDrRMDIgajBUcbCS6aLGQVgGUVYxMcCJWKwlSr4A5PVoEHyCrdlZNTAA9IMwbhqamhj5ojJTGGDAGNlqGFCcamLwyJ1YRBUQ1B3eYdb9tuDxR0m+js6hHtvHVNUxKIwARACM7wA02QBGjFaACZelZ9NZbA0IAA6YBmGBAA

[hosseinmd]

I am actually wondering if it's possible to not have to specify RuleNames, because technically they are already there in the object and TS could use those keys and map them to classes

Yes, it is possible, if we move ruleNames to last generic param, so we just need to define props and ruleNames will be recognized

[kof]

What version of TS are you all testing with? Just to make sure this is not the source of our differences

[hosseinmd]

What version of TS are you all testing with? Just to make sure this is not the source of our difference

I testing now, give me time.

[hosseinmd]

@ITenthusiasm is right, RuleNames not inferred

The benefit of this PR is

//change this line 
const themeArg3 = createUseStyles<string, MyProps>
// to this line 
const themeArg3 = createUseStyles<MyProps>

[hosseinmd]

Sorry this PR is wrong.

[ITenthusiasm]

but I am guessing it's not representing the actual problem

@kof Yeah that's not quite the same as the actual problem. There are a few reasons why, but I don't think diving into those reasons would advance the React-JSS discussion much more, and I don't want to write another chunk of text. @k-yle added a really good TypeScript playground to reflect the issue (thanks). I don't think a more relevant example can be added than his.

If someone wants to see a basic example of the issue I was describing, microsoft/TypeScript#43119 is a sufficient example. If someone wants to see an example of this issue in the context of React-JSS, then k-yle's TS playground is a sufficient example.

this will be maybe a good example to explain for future readers

If we're talking about documenting things for future readers, then honestly, my first comment on the matter and k-yle's are enough. From this discussion and PR alone, there clearly isn't a "simple" or "easy" way to explain the problem in 2 sentences. And a random isolated example isn't going to make sense without the background explanation.

What's also clear is that many people will likely be unwilling to read something as large as a condensed explanation anyway, especially when they're running with (understandable) presumptions about how TS should operate. And if that's the case, no amount of talking or isolated examples will help them. People will just have to run into the problem themselves and bang their heads against the wall like I had to do. They'll learn the truth one way or another by playing with the source code or k-yle's playground.

I can't help anyone further than what I've said. And at this point I've given the same explanations and set of references at least 2-3 times between this PR and #1460.

[kof]

@ITenthusiasm yeah, totally understandable, it must be frustrating, consider this as teaching us

I haven't used typescript yet (at the job still flowtype), will soon start using it eventually though.

I played a bit more and found this workaround as alternative to typing out the list of rule names:

https://www.typescriptlang.org/play?jsx=0&ts=4.2.3#code/C4TwDgpgBAyqA2EBKBXRUC8UCGA7EA9AUhAMYD2ATgCYA8A5BfFfQDRQDOwlAlrgOYA+ALAAoMRVxcowcgGF42DhwgdMUWnBCIOggBSlFy1QC4oAbygBtVIgBy2ALbQ+UANYQQ5AGawEqgF0zLl4BKABfAEpMQShDJRUOAG4xCXIpYDjyRzAeRC1EW2gsWgAFSnIwXT0ubWQ0CDMC+sR2MAqqs3LKtQAfHHxIpv8imIsxKCgeXz16DmyIbrB6Kdwodp7o8wnJ9Y6OADp55yWU8VFJ8J3KCGAUSjXawoaxK-PJaW8Kx2aOABV5EZEuoyvt2L99E9TH46hw2vsuvtohhYtsLlkMnEgdC8CB1OY3pNvFQoAZ0tJKA0HM5Vpx-BwtjtJh9MlDRlgoRwrJT7E4IAEmViEqpuVS+QF1BQcnkIM0iqCepCRg14ZsdoSoEQoH9ePx+BBKKtvAaILhSBBrrd7mtZAphRxNPT9PFjAzXql3uTWfT8TtsGY0bsssxKGZ6DdqPR1awdgAjAOCphUMP8G6mqPot5vMSgSBQJZqLAkCg0BjHRYdNhQXAoRyxg0iT2YjZVfHlpZmACM2fORBZQtd6i+2V+ALtrpq9NVVUiaUxLuBWGHP3pY+xDoLSth07dvYInAAFuQ0NR1gkJOuDtgklAPVqOEeT9XyJkwOfRAvVAdSDexEA

the idea here is to use typeof styles at the call site so that consumer doesn't have to define rule names list

[kof]

closing this PR as we figured this out

[kof]

Holy s. this thing falls apart as soon as I try declaring styles as a separate object, Props doesn't get used any more for function values