v3

[garronej]

The remaining question is: How to we make this work with withStyles.

[garronej]

Hi @olee, what do you think about an API like the following one where we provide the refs as type only instead of passing an actual argument?

const useStyles = makeStyles<{ color: "red"; }, "child" | "child2">()((theme, { color }, refs) => ({
        "parent": {
            "padding": theme.spacing(3),
            [`&:hover .${refs.child}`]: {
                "background": color,
            },
            [`&:hover .${refs.child2}`]: {
                "background": "red",
            },
        },
        "child": {
            "background": "blue",
        },
        "child2": {
       }
}));

This could potentially unlock the situation we have with withStyles making the refs being inferable from which @mui is passed.

It's hard to achieve but using es5 Proxies we could theoretically pull it off. Proxy can be polifyied for IE.

[olee]

Why do you think this won't work with withStyles?
You can just use it the same way like this:

const MyComponent = withStyles((theme, props, refs) => ({
    parent: {
        padding: theme.spacing(3),
        [`&:hover .${refs.child}`]: {
            background: props.color,
        },
        [`&:hover .${refs.child2}`]: {
            background: "red",
        },
    },
    child: {
        background: "blue",
    },
    child2: {
    }
}), { name: 'MyComponent', refs: ['child', 'child2'] })(MyComponent);

If you check material-ui v4 withStyles you will actually see that it also has that options object as a second parameter:

[garronej]

Yes it could work but the difference between makeStyles and withStyles is that in withStyles the rules names are defined by the component passed as argument.
Take the mui <Button> for examples the rules names are "root" | "text" |"textInherit"...
The refs array element type would have to be a subset of theses rules.

It would be much cleaner if we could make it work without requiring the user to specify an information we already have...

[garronej]

Hi @olee,
I implemented, tested and documented the new API. I think it's ready for v3 🥳
I'll wait some days before releasing to see if you think there is things that needs to be changed.
Sorry I havent got the time to implement en beta release mechanism yet.
You can read at the top of the readme the breaking changes.

Thank you for your help, Best

[olee]

Yeah I actually got my workplace to give me time to take care of this PR as we will be using this productively.
So if you can wait for this a little bit then I will be able to look into this properly at least until the end of next week 👍

[garronej]

I updated our CI framework to be able the publish beta versions.
There is now a beta version of tss v3 available on NPM.
I put it in production with our stuffs. I think it's ready for release.
I will however leave you some time to test and review before merging 👍🏻

[olee]

The counter should still be global imho, as creating multiple makeStyles when used by libraries etc. can still create duplicates. I think there is no issue in just moving up those few lines to be outside of the makeStyles function.

[garronej]

@olee alright! Thanks for the review. It's live.

[olee]

What about the other review comments? 😅

[garronej]

@olee Sorry have I missed something?
I looked everywhere but and found nothing.
I maybe erased some stuff with a rebase?

[olee]

There's a bunch of review comments I created:

They are just above here in the feed 😕

[garronej]

Your review is pending, I can't see it yet 🙂

[garronej]

Thanks, good catch!!

I'm always using " for property when declaring an object. It help quickly see if we are declaring a type or creating an object. It improves a lot TypeScript code readability. 👍

I published a new release.

[olee]

I'm not sure if using a proxy is super smart or actually a bad idea here...
While on the one side it allows not having the define the ref names prior to the styles, it makes it necessary to provide explicit generic arguments to the makeStyles invocation which is quite annoying and it also provides no safety at all for non-typescript use (totally optional imho as pretty much everyone should use TS, but it's still a thing to consider).

[garronej]

explicit generic arguments to the makeStyles invocation which is quite annoying

Passing { "ref": [ "a", "b", "c" ] } if anything is more verbose that "a" | "b" | "c". The only thing that pain me is to have to explicitly provide void as the Param argument. makeStyle<void, "a" | "b" | "c"> 😔

it also provides no safety at all for non-typescript use

Making explicitly provide the "ref": [ "a", "b", "c" ] provide no safety to the JS users. If anything it's one more place where they can screw up. In JSS they where used to $ruleName without having to provide an extra parameter. They expect the same with TSS.

There is also two extra argument in favor of using type only param instead of runtime param:

• For withStyles() (API that I personally don't use but that is important to many non the less) combined with MUI components, it's a game changer:
  https://user-images.githubusercontent.com/6702424/143791304-7705816a-4d25-4df7-9d45-470c5c9ec1bf.mp4
• It make writing the codemod for automatic migration from v4's makeStyles and withStyles achievable.

[olee]

Well as mentioned before already, withStyles also has a third argument to pass options so that should not be an issue

[garronej]

Sorry I provided the wrong gif.
I already explained it to you. Why would you require user to explicitly pass in an information we don't need and we already have?
It would be a fundamentally flawed design.
In withStyles, it's not the user that get to decide the rules name. They are defined by the type of theclasses props.

[olee]

Oh man... I didn't know I had to specifically "submit" a review because I never did this 😣

[garronej]

Thank you very much for taking the time to review the code so thoughtfully.
Even if I don't agree with you on all the coding style aspect, your contribution is very much valuated.
Without you several important bugs would have remained unnoticed, we wouldn't have labeling and there would still be stuck with the createRef() API.

Many thanks.