Requirements for making tss-react compatible for Material-UI makeStyles replacement

[garronej]

Requirements for making tss-react compatible for Material-UI makeStyles replacement

[clytras]

Well, I am very passionate about this topic. Let me make my case and if you aren't convinced I'll make the API changes, even if I think it's a mistake, because I'd like this colab to go through.

First, I believe it's almost always better to return results of functions wrapped into an object, even if there is only one result. The reasoning behind that is that a large part of a developer's job is to give meaningful and consistent names to things.
By providing the results with a default name, we switch the responsibility of finding a suitable name from the API users to the API designer.
This is huge for consistency and quality of life for developers.
What happen in practice if we don't do that is that users either refer to documentation examples every time or come up with uninspired names.

No, it's not right to return always an object when we want to return a single value; IMO this is a bad practise. End users must have a direct freedom to name things as they want; we should not create things just having in mind beginners that will make up weird names, teams and experienced developers will always be consistent regarding the names they're using. A library shouldn't care what internal names a project shall use. Most users and especially beginners, will always read the documentation when they'll start to learn how to develop (using MUI for example) so they'll see what naming convention the framework is using.

An other awkward thing about this, is when we want to create multiple useClassNames or multiple useStyles, then we're forced to do ugly destructuring renaming like:

const { useClassNames: useComp1ClassNames } = createUseClassNames(...)

const { useClassNames: useComp2ClassNames } = createUseClassNames(...)

const { useClassNames: useComp3ClassNames } = createUseClassNames(...)

instead of:

const useComp1ClassNames = createUseClassNames(...)

const useComp2ClassNames = createUseClassNames(...)

const useComp3ClassNames = createUseClassNames(...)

Libraries should not be opinionated regarding naming conventions and should not enforce any king of end naming.

[garronej]

Hi @mnajdova,

SSR

Have you tried to configure nextjs to work with tss-react? It uses @emotion/css so I believe some extra steps would be required. This would be the first requirement so that we could use it together with material-ui. Our docs (the Material-UI's) can be a good candidate for testing this out.

I haven't tested it yet. I can make time for that on the week-end.

Is this still true?

Question: Is it really necessary to invoke function createUseClassNames() and then propagate the input to the next function call? What are the arguments required there? Can it be omitted?

Yes, it unfortunately is necessary in order to get the type inference working.
The root of the problem is that TypeScript doesn't support partial argument inference.
In other word we can't have a generic function with two type argument, specify one and let the other be inferred.
Consider this example:

 createStyles<"root" | "label", { color: string; }>({
    "root": {
        "backgroundColor": "blue"
    },
    "label": ({ color })=>({
        color
    })
})

We shouldn't have to explicitly provide "root" | "label" it should be inferable from the object passed in argument.
But because we have to specify { color: string; } we have to put "root" | "label" as well.
The only way to circumvent this limitation is to use the higher order function pattern as implemented in tss-react.

API suggestions

Here are some suggestions regarding the API itself:

I wouldn't return named options, if there is only one option returned. For example:

-const { createUseClassNames } = createUseClassNamesFactory({ useTheme }); // createUseClassNames is the only option
+const makeStyles = createUseClassNamesFactory({ useTheme });

-const { useClassNames } = createUseClassNames()(
+const useStyles = makeStyles()(
  (theme)=> ({
    root: {
      color: theme.palette.primary.main,
    },
  })
);

With this, we can actually make the API closer to the v4 makeStyles API. We could overcome this by adding adapters, but I will leave it to you.

Well, I am very passionate about this topic. Let me make my case and if you aren't convinced I'll make the API changes, even if I think it's a mistake, because I'd like this colab to go through.

First, I believe it's almost always better to return results of functions wrapped into an object, even if there is only one result. The reasoning behind that is that a large part of a developer's job is to give meaningful and consistent names to things.
By providing the results with a default name, we switch the responsibility of finding a suitable name from the API users to the API designer.
This is huge for consistency and quality of life for developers.
What happen in practice if we don't do that is that users either refer to documentation examples every time or come up with uninspired names.
On the other hand, this kind of flow is only possible with named returns:

Maybe it is more critical for me than for others because I am dyslexic but I know I am knot the only one for whom this pattern is life changing.

Beside I am convinced that we should ditch the old naming scheme: makeStyles -> useStyles -> classes it is arbitrary and inconsistent, createUseClassNames -> useClassNames -> classNames make much more sense although being a bit longer.
I understand the willingness to keep the naming familiar but users will have some refactoring to do if they want to switch from the V4 hook API to TSS. It's now or never the time to ditch the legacy naming scheme.

Best regards

[mnajdova]

SSR
Have you tried to configure nextjs to work with tss-react? It uses @emotion/css so I believe some extra steps would be required. This would be the first requirement so that we could use it together with material-ui. Our docs (the Material-UI's) can be a good candidate for testing this out.

I haven't tested it yet. I can make time for that on the week-end.

I will set up a simple repository to test this out, and we can see if there are some problems. From the initial try on the Material-UI docs it did not work, but it would be simpler to reproduce on a clean slate.

Is this still true?

Yes

Yes, it unfortunately is necessary in order to get the type inference working.
The root of the problem is that TypeScript doesn't support partial argument inference.
In other word we can't have a generic function with two type argument, specify one and let the other be inferred.
Consider this example:

 createStyles<"root" | "label", { color: string; }>({
    "root": {
        "backgroundColor": "blue"
    },
    "label": ({ color })=>({
        color
    })
})

We shouldn't have to explicitly provide "root" | "label" it should be inferable from the object passed in argument.
But because we have to specify { color: string; } we have to put "root" | "label" as well.
The only way to circumvent this limitation is to use the higher order function pattern as implemented in tss-react.

It's unfortunate that the API needs to suffer because of TypeScript limitation :( In Material-UI we have a similar createStyles function https://github.com/mui-org/material-ui/blob/next/packages/material-ui-styles/src/createStyles/createStyles.d.ts but it can be used optionally: https://next.material-ui.com/guides/typescript/#using-createstyles-to-defeat-type-widening Would be great if we can do something similar here. I believe it is used for the same exact reason.

Regarding the API changes :)

Beside I am convinced that we should ditch the old naming scheme: makeStyles -> useStyles -> classes it is arbitrary and inconsistent, createUseClassNames -> useClassNames -> classNames make much more sense although being a bit longer.
I understand the willingness to keep the naming familiar but users will have some refactoring to do if they want to switch from the V4 hook API to TSS. It's now or never the time to ditch the legacy naming scheme.

I understand your point, looking from Material-UI perspective, if the API stays as it is at the current moment, we will need to create an adapter so that we can keep the same API as in v4. We are considering adding back the makeStyles partially so that we can ease up the migration (together with the other benefits it has), so adding more changes to it, won't help with it.

If there are other reasons for adding adapter, then it may be fine, but int he end it would be great if we can keep the same API :)

---

Will link here later today/tomorrow the repository where we can test the integration.
Edit: Here is a link to the repository - https://github.com/mnajdova/mui-tss-react-integration I've created one issue that we need to resolve.

[mnajdova]

@garronej I feel like we can close the issue, I don't see some blockers at this moment. As mention in mui/material-ui#26571 (comment) we are going to promote tss-react as the emotion's equivalent to the makeStyles API. If other developers find issues, it would be better to open new ones that will be more focused on a specific problem.

[garronej]

Well... we have conflicting views.
I think the DX is better if we can deconstruct the returned value. I prefer not having to wonder, "what is this supposed to be called again?" it distracts me from the important things.
Beside it doesn't enforce the end naming, it's just a suggestion, you find the syntax for renaming awkward, I'll admit it's a bit verbose but a vast majority of people will always want the standard name, especially for a hook that have to start with use.

That's said, I am willing to make the following API changes if there is a clear consensus in this direction.

-const { useStyles } = makeStyles()(...);
+const useStyles = makeStyles()(...);

@clytras, on a personal note, your peremptory tone did rub me the wrong way. Feel free to express your opinion, it is taken into consideration but please, be polite.

[clytras]

@garronej what exactly wasn't polite and with a "peremptory tone" regarding my comment? That I told you that "it's not right to return always an object when we want to return a single value" or "IMO this is a bad practise"?

I'm trying to make you rethink of something that it's obsiously wrong; Yeah, maybe I should have written "please reconsider the usage of this BAD practise" but I believe you're getting the point and I have no intention to insult you nor offend you in any way.

It's not a hook, it is a function that it creates a hook and that makes a huge difference because it's very likely that there might be multiple hooks in the same file, it's not a standard hook like useState or useEffect; people will use a function to create it and they should have the freedom to name it without the need of extra destructuring syntax.

There are cases that we need to return an object even if we have a single value to return, like for example the useRef hook that it always return an object because we need that ref of that object to be immutable while to hold something in the current property that it shall be mutated. In the makeStyles hook generator we don't have such a need.

I prefer not having to wonder, "what is this supposed to be called again?" it distracts me from the important things.

Developers won't have to wonder when they see how it's done in the examples and tutorials which they will dive, read and follow when they are about to learn something. IMO creating such an opinionated behaviour only adds syntactic noise and it doesn't add much regarding DX.

you find the syntax for renaming awkward, I'll admit it's a bit verbose but a vast majority of people will always want the standard name

It is awkward when I want to create 3, 4 and many hooks to destructure a repetitive name like this; remeber here we don't have a standard hook, but we're creating new hooks and there might be many of them.

Please reconsider the usage of this and please don't take my words personal; I don't know you, and there is no intention for any offence.

[garronej]

I shouldn't have took your words personally, as I said I am very passionate about this issue. 😅

It's not my place to assert how the lib should be used maybe you have a valid use case where it make sense to declare multiple useStyle side by side, it's just that I don't see it, maybe if you link me the code it would convince me.

Also, I stress that named return is an accessibility features. We are not all equals regarding what we can remember, a lib that only requires to remember concepts and not naming conventions is way more friendly to people with ADHD and beginners.

That said your point was upvoted twice, @mnajdova also suggested this change and it was like that in @material-ui v4 so I've done it:

This is a tiny but helpful package. It would be great if we can integrate it as one of the default style options in MUI, applying exactly the same interface as what JSS supports in V4 and keep maintenance the code within MUI repo.

[garronej]

OK there is something that I want to point out.
The API of TSS and the one of makeStyle are different beyond the naming conventions.

TSS:

const { useClassNames } = createUseClassNames<{ color: string; }>()(
  (theme, { color })=> ({
    "root": {
        "backgroundColor": theme.palette.primary.main
    },
    "label": { color }
  })
);

makeStyles:

const useStyles = makeStyles(
  theme => createStyles<"root" | "label">, { color: string; }>({
    "root": {
        "backgroundColor": theme.palette.primary.main
    },
    "label": ({ color })=>({
        color
    })
  })
);

Notice how with TSS we get color alongside the theme rather that with makeStyles we get it as argument of the label property. I made this change so we don't need the createStyle anymore.

I believe it is used for the same exact reason

createStyles was introduced to cope with the fact that the API, as designed in JSS is hard to type right. Notice however that createStyles doesn't solve the problem of having to enumerate the inferable "root" | "label".

It's unfortunate that the API needs to suffer because of TypeScript limitations :(

Regardless, I think we should optimize the user experience for TypeScript, not for vanilla JS.

In Material-UI we have a createStyles would be great if we can do something similar here.

...we can't, it's technically impossible as of TypeScript v3.4.5

If there are other reasons for adding adapter, then it may be fine, but in the end it would be great if we can keep the same API :)

Actually, there is one. Currently unlike JSS the TSS has no support for composition, see this issue. I am planning to implement this proposal later this week. As crateUseClassName will return more than one item, I have to keep the wrapper pattern.

Best regards.

[garronej]

We now have a polyfill the css function of @emotion/css using @emotion/react.

Meaning that TSS will be able to reuse the version of emotion bundled in @material-ui/core v5.

Focusing on getting things to work on SSR. @mnajdova you have been of great help so far.

Best regard

[oliviertassinari]

This PR https://github.com/DimensionDev/Maskbook/pull/3745/files gives an interesting overview of the feasibility to move from @material-ui/styles to tss-react.