Improve Types for Hooks

[ITenthusiasm]

What Would You Like to Fix/Add?

This PR aims to ensure that the createUseStyles hook has accurate TS intellisense for props and themes. The details of the work can be found in the commit message. Note that as a result of cascading effects, this PR also improves the types for the deprecated withStyles HOC. This should satisfy #1273 and #1431.

These are TypeScript changes only, as only these changes will be visible to the TypeScript users. These are the minimal changes necessary to improve the user experience for TS devs. Flow is separate and unrelated to the user, and I'm not so familiar with it, so it's simplest to add flow changes in a separate commit (if that's acceptable). (If someone else familiar with Flow could apply the changes, that would be helpful. I've already supplied the TS types, so it should be easy.)

Goals

• The types should be accurate for both kinds of arguments supplied to createUseStyles/withStyles (ie. the object argument and the function argument).
• The type definitions should ban the declaration of [React-JSS-related] types that have already been defined in the surrounding scope. This prevents confusion that could be caused from unnecessary shadowing (which many eslint configs would oppose anyway). It also discourages users from attempting to define nested functions (which JSS seems to warn against already).
  • As an example: A function of props and theme that returns a style object will not give type inference to nested functions.
• The generic types should be ordered according to what users are most likely to need or use. For instance, Props is typically more necessary than Theme.
• A withStyles "lazy migration" in TS should be "sufficiently easy" ("not too complicated") to write given these changes. If not, these new types may need to be revisited. (This requires closely examining how well the HOC is defined, not just how well these new types are defined.)
  • On that note, I intend to send out a PR for this in the future, pending this PR. See [docs] Improve Help for People Doing a Lazy Migration for HOCs #1459.

Caveats

• Supplying any generic type parameters causes all defaults to be used by TS. This means you risk losing the implicit definitions of class names whenever you specify props/data.
  • This is a TS problem, and it will hopefully get resolved in the near future. See Implement partial type argument inference using the _ sigil microsoft/TypeScript#26349.
• No breaking changes for JS. But for TS (mainly React), if anyone is using the old createUseStyles<Theme> syntax, they will have to update their types. This is unfortunate, but not hard. And since TS has better intellisense when Name is not specified, this shouldn't be a common concern.

Unknowns / Concerns

• Resolved: Use default any. In jss/src/index.d.ts, should all default usages of JssStyle be updated to use JssStyle<any> in the context of these proposed changes?

Extra Notes

• createStyleSheet uses any for the Data/Props type because the data can be dynamically changed at any point. any is safest.

Todo

• Add tests that verify the modified behavior
• Add documentation if it changes the public API
  • N/A (no TS documentation currently exists...yet)

[ITenthusiasm]

Just saw the CI fail due to some peculiar browser issue. But I didn't touch any JavaScript. 🤔 Is this something to be worried about?

[ITenthusiasm]

From Ryan's Comment on microsoft/TypeScript#39008, it unfortunately seems like <Name, Props, Theme>, may actually be a more practical ordering of the type parameters, even though it isn't necessarily in order of "relevance". 😔

Such a change would also remove one of the caveats I mentioned while getting things more ready for what microsoft/TypeScript#26349 would hopefully bring to the table soon.

Thoughts? I could apply the change if desired.

[moshest]

Overall I liked that a lot. Great job!

[ITenthusiasm]

I'm a little preoccupied right now. But just in case, really quickly, I wanted to call attention to:

• Resolved: Use default any. In jss/src/index.d.ts, should all default usages of JssStyle be updated to use JssStyle<any> in the context of these proposed changes?
• Resolved: Use the Styles<Name, Props, Theme>. Styles<Props, Theme, Name> ordering vs. Styles<Name, Props, Theme> ordering.

Some thoughts on those would be helpful before merging (if everything gets approved). I know moshest commented on the default type for JssStyle. If the default is changed accordingly, then the first item in the list would automatically be resolved.

[ITenthusiasm]

Rebased onto master.

[k-yle]

This is great (: I've left a few comments

[ITenthusiasm]

Just for the record, I intend to clean up the commit history with a local rebase once everything is finalized. So once all the conversations are resolved -- if the PR comes out to be acceptable -- please don't merge the PR until I've reduced it to 1 single commit.

[ITenthusiasm]

Since everything got approved sooner than I expected, I reduced my commits today (instead of waiting until tomorrow).

These are not breaking changes for JS. However, they are breaking changes as far as TS is concerned due to the new type definitions (particularly for createUseStyles).

The PR is approved and the history is reduced to a single commit. @kof, if these changes are acceptable, then this PR is ready to merge. Otherwise, if there are any changes you or anyone else would like me to add, I'd be happy to include them.

[kof]

Is it acceptable to release this under minor version increase?

[ITenthusiasm]

Is it acceptable to release this under minor version increase?

@kof, The short answer is that I'm not sure. But in my opinion, a minor version increase is probably fine. I mainly feel this way since the JS API hasn't changed, and this is an improvement on the TS types, where features were previously missing.

How do other TS projects handle such changes, as breaking change and major version increment?

I tried to do some research on this but didn't find any "objectively right" answers. I saw #1255, where some users were upset about breaking type changes. So...some people would likely get upset with a minor increase. But I also saw your link to Ryan's comment on breaking type changes at the end of that issue. Ryan has additional comments that may be related to the matter here and here. Additionally, other libraries like CRA seem to release minor versions with breaking changes if the changes aren't expected to be too significant for most users.

I also noticed your comments on #1351. If the types truly haven't stabilized yet, then it may be practical to forego bumping the major version since complaints/issues related to types would be found often. Technically speaking, if the types were back on DT, users would encounter breaking changes without bumping major versions anyway. (Btw, I think the types are better housed here than on DT.)

If there aren't any large incoming JS changes, then bumping the major version only for this TS change (especially if the definitions haven't fully stabilized yet) feels a bit weird. I'm sure there'll be people who disagree. But after observing a few discussions -- and just thinking about it practically -- I find an overly religious following of semver unhelpful. (I do like semver though. And generally speaking, it's practical and desirable.)

Personally, I support going with a minor release. If this does happen to be released as a minor change, I'll take the heat of the complaints. The decision is up to you though. (Others are free to chime in.)

Warnings about breaking changes in a changelog, joined with a useful migration guide, would probably facilitate a minor release. It would hopefully reduce complaints too.

[ITenthusiasm]

If anyone's interested in an opinion besides my own on the versioning, I saw this comment from a moderator in the Testing Library Discord.

I'd say it's a patch if the typing before was wrong. If you narrow the parameter types along the way, it should be major.

I couldn't find this person's GitHub, so I don't know who they were. But it's another thought!

If strictly, religiously following semver is the goal of this project, then a major version would likely make more sense.

But again, given the volatility of the types, that may cause unnecessarily frequent major bumps (unless the types stabilize some day).

[ITenthusiasm]

No rush on the PR, but I'm curious to know what the intentions with this PR will be? My team would benefit from being able to employ the new type changes (as would others). And I don't feel comfortable starting on the work for the lazy migration HOC docs (TypeScript) until I know this is finalized.

I'm just trying to get an idea of the timeline.

[kof]

@ITenthusiasm I am finishing #1456 and then gonna merge this one and make a release with all recent changes as a minor

[ITenthusiasm]

There was a line or 2 where some previous changes were accidentally reverted. My recent amend to the commit fixed those.

[kof]

Can you please add to changelog whatever we have done here? eventually with hints what needs to be done in case of errors

[ITenthusiasm]

I can do that. Would you want me to update the changelog in this PR or in a separate one?

[kof]

Both works, if you want to create a separate - feel free

[ITenthusiasm]

Okay cool. I'll probably create a separate PR in that case. I'll try to add changelog updates today or tomorrow.

[kof]

Merged 🎉 Thank you everyone!

[fardad-dev]

Order of generic should be changed.

Current is

createUseStyles<C extends string = string, Props = unknown, Theme = Jss.Theme>

should change to

createUseStyles<Props = unknown, Theme = Jss.Theme,C extends string = string>

[ITenthusiasm]

Hi @hosseinmd. Originally, this PR had the order you suggested, but it was changed to the current ordering because it plays most nicely with TypeScript.

Due to the nature of TypeScript's type inference, putting C last actually causes users to lose type info. More specifically, if any generic type parameters are specified, then TypeScript immediately assumes the defaults for the remaining, unspecified type parameters. So for instance, under your suggested order, this

const useStyles = createUseStyles<MyProps>({ /* ... */ });
const classes = useStyles(props)

would cause classes to be typed as Record<string, string> instead of Record<RULE_NAMES, string>. With this knowledge, there are 3 main problems:

1. Under createUseStyles<Props, Theme, C>, anyone who only wanted to use Props will now be forced to supply a type for Theme so that they can finally supply a type for C (to get a proper classes type). This would result in a lot of code looking something like createUseStyles<Props, never, C>, which is unappealing and inefficient. Unfortunately, users will always have to specify C if they want autocomplete on classes, so C actually takes the highest priority.
2. Under createUseStyles<Props, Theme, C>, we would not be able to take maximum advantage of Implement partial type argument inference using the _ sigil microsoft/TypeScript#26349 if/when it gets merged. With createUseStyles<C, Props, Theme>, users would be able to do createUseStyles<_, MyProps> to maintain an accurate classes type. It's not flawless, but it's the best arrangement available that would reduce rule name duplication.
3. Under createUseStyles<Props, Theme, C>, we would very likely get more issues reporting "bugs" in React-JSS that are not React-JSS bugs at all but are in fact misapplications of TypeScript. The TypeScript limitation I described to you earlier is not readily apparent to the average TS user, nor does it seem to be a frequently encountered problem. Consequently, when users see that classes is properly typed when Props isn't specified, and that classes loses types when Props is specified, many will (understandably) conclude that the issue is in React-JSS. A similar problem occurred in Loosing generic type of component while using withStyles() HOC  #1331, where a TS issue was mistaken for a React-JSS issue. Of course, we should design around user experience, not simply minimizing "bug" reports. But since we get both with <C, Props, Theme>, this is a more valuable order.

[fardad-dev]

I don't agree with you at all. C generically defined from inserted object to createUseStyle, and usually we don't need to specify C

[ITenthusiasm]

Friend, you're free to disagree. But if you closely read the PR, the comments, and the issues I mentioned in my last comment, you would understand that TypeScript will not work correctly if C isn't specified. Your response tells me that you did not closely examine the examples I gave nor the issues I linked. I can't help you beyond the linked issues and examples I provided (additional examples exist in the linked issues).

Trust me, I ran through tons of headaches trying to figure this thing out. The current order is due to a limitation in TypeScript.

[fardad-dev]

TypeScript will not work correctly if C isn't specified

This is not true.

[fardad-dev]

My application typescript is broken, and I have to define whole C and props, but we don't need to define C because that could be recognized from inserted object.

[fardad-dev]

I have to back old version

[ITenthusiasm]

@hosseinmd I'm unwilling to continue the conversation until you observe the issues I linked and the evidence I provided.

I'm the one who created the PR, which -- as I mentioned -- started off as createUseStyles<Props, Theme, C>. Consequently, I'm also the one who saw classes lose type inference when running the extensive tests that I wrote. Though C does not lose inference in all situations, it does lose inference once any generic type parameters are provided. This is what led me to create one of the TS issues I linked (which is now closed due to the _ sigil PR they have open). This is also what led me to place C at the front before merging.

My application typescript is broken

Because Props was not a previously provided type parameter, a break would have occurred anyway under your suggestion. So this comment confuses me slightly. If you were never supplying Props (or Theme), your application shouldn't be broken right now. If you're concerned because you wanted an easy way to supply props, you can see #1467. It provides examples for implicit and explicit typing. If you're concerned because of issues with Theme, see the migration suggestions in the changelog. If you'd be helped by defining a global Theme type, see #1453.

[fardad-dev]

I created a PR please review that work well

[MaximeCheramy]

I've updated react-jss and I'm now facing this issue: #1479