[feature] Auto-generate label property in makeStyles

[garronej]

[feature] Auto-generate label property in makeStyles

[olee]

Personally I do not think of it as a requirement to make css names follow the component name.
It is what devs will usually do, but in general this is nothing more than a hint to show where the styles came from.
I could imagine allowing to use the component function as name option so it would take MyComponent.name if it is available, but for me this would be a totally optional thing.

I am also working on another improvement to the API regarding ref usage which I think are really nasty with the current API and can be simplified a lot 👍
This API change would also make use of the option argument in makeStyles which would go really well with the name addition.

[garronej]

As for the bug: look here

Fixed in 1.0.3

[garronej]

Hi @olee,
Seems like a nice addition!
My concern is about maintainability.
I want to make every effort possible to only push API that'll generate dry and maintainable code.
With your proposal, if the component gets renamed we have to remember to update makeStyle({ "name": ... }) as well.
I would like to leverage something like this.
It could let us write:

import { makeStyles } from "...";
import { symToStr } from "tsafe/symToStr";

const useStyles= makeStyles({ "name": symToStr({ MyComponent }) })(
    theme=> ({})
);

function MyComponent(){
    //...
}

or even, if we use symToStr internally:

import { makeStyles } from "...";

const useStyles= makeStyles({ MyComponent })(
    theme=> ({})
);

function MyComponent(){
    //...
}

The problem is if we use react memo:

import { makeStyles } from "...";
import { memo } from "react";

const useStyles= makeStyles({ MyComponent })(
    theme=> ({})
);

const MyComponent = memo(()=>{
    //...
});

it will crash because MyComponent is declared after makeStyles() is invoked...
idk if it's reasonable to expect users to move their styles under the component...
After all why not? It's the costume in react native, we could then write something like:

import { makeStyles } from "...";
import { memo } from "react";

const MyComponent = memo(()=>{
    const { classes } = useStyles(); 
    //...
});  

const useStyles= makeStyles({ MyComponent })(
    theme=> ({})
);

What are you though?

[garronej]

Personally I do not think of it as a requirement to make css names follow the component name.

It will be possible to pass either a string or a wrapped component.

I could imagine allowing to use the component function as name option so it would take MyComponent.name

It won't work with memo and memo should always be used.

I am also working on another improvement to the API regarding ref usage which I think are really nasty with the current API and can be simplified a lot 👍

Ok, I'll wait and see.

[olee]

Ok I think I pretty much got it, even with the simplified ref usage (which should also fix a potential bug which I will mention later):

Implementation

import { useCssAndCx, CSSObject } from 'tss-react';

export interface MakeStylesOptions<RefName extends string = never> {
    name?: string | Function;
    refs?: RefName[];
}

export type StyleRules<RuleName extends string> = Record<RuleName, CSSObject & { [k: string]: unknown; }>;

export type StyleRulesOrCallback<Theme, RuleName extends string, Params = void, RefName extends string = never> =
    StyleRules<RuleName> |
    ((theme: Theme, params: Params, refs: Record<RefName, string>) => StyleRules<RuleName>);

let refCount = 0;

/**
 * Advanced version of makeStyles from tss-react which generates readable, debuggable class names and has easier ref usage
 * 
 * @see {@link https://github.com/garronej/tss-react}
 */
export default function createMakeStyles<Theme>({ useTheme }: { useTheme: () => Theme; }) {

    /** Hook which returns theme as well as css and cx functions */
    function useStyles() {
        const theme = useTheme();
        const { css, cx } = useCssAndCx();
        return { theme, css, cx };
    }

    function makeStyles<Params = void, RefName extends string = never>(opt: MakeStylesOptions<RefName> = {}) {
        let {
            name,
            refs = [],
        } = opt;

        // Extract name from function object
        if (typeof name === 'function' && typeof name.name === 'string') {
            name = name.name;
        }
        const labelPrefix = name && typeof name === 'string' ? `${name}-` : '';

        // Generate ref map and also validate passed ref names
        const refsMap = Object.fromEntries(refs.map(ref => {
            if (/[\s!#()+,.:<>]/.test(ref)) {
                throw new Error(`Illegal characters in ref name "${ref}"`);
            }
            return [ref, `tss-react-ref-${labelPrefix}${ref}-${refCount++}`];
        })) as Record<RefName, string>;

        return function <RuleName extends string>(stylesOrFn: StyleRulesOrCallback<Theme, RuleName, Params, RefName>) {
            const stylesFn = typeof stylesOrFn === 'function' ? stylesOrFn : () => stylesOrFn;

            return function useStylesHook(params: Params) {
                const theme = useTheme();
                const { css, cx } = useCssAndCx();

                const styles = stylesFn(theme, params, refsMap);

                const classes = Object.fromEntries(Object.entries<CSSObject>(styles).map(([ruleName, cssObject]) => {
                    // Assign label to class name if none is provided
                    if (!cssObject.label) {
                        cssObject.label = labelPrefix + ruleName;
                    }
                    // Transform css object into class name
                    return [ruleName, css(cssObject)];
                })) as Record<RuleName, string>;

                // Return all data
                return {
                    classes,
                    theme,
                    css,
                    cx,
                };
            };
        };
    }

    return { makeStyles, useStyles };
}

Examples:

Usage with name

const useStyles = makeStyles({ name: 'MyComponent' })({
  root: { /* ... */ },
});
const expectedResult = {
  root: `tss-react-[hash]-MyComponent-root`
};

Usage with component function for name (not recommended imho!)

function MyComponent(props) { 
  return <div {...props} />;
}
const useStyles = makeStyles({ name: MyComponent })({
  root: { /* ... */ },
});

Simple usage with refs

const useStyles = makeStyles({ name: 'RefsExample', refs: ['small'] })((_theme, _p, refs) => ({
  root: { /* ... */ },
  small: {},
  header: {
    height: 50,
    [`&.${refs.small}`]: {
      height: 30,
    }
  },
  container: {
    padding: 32,
    [`&.${refs.small}`]: {
      padding: 16,
    }
  },
}));
const expectedResult = {
  root: `tss-react-[hash]-RefsExample-root`,
  small: `tss-react-[hash]-RefsExample-small tss-react-ref-RefsExample-small-0`,
  header: `tss-react-[hash]-RefsExample-header`,
  container: `tss-react-[hash]-RefsExample-container`,
};

Advanced usage with refs

const useStyles = makeStyles({ name: 'RefsExample', refs: ['small'] })((_theme, _p, refs) => ({
  root: { /* ... */ },
  smallSize: {
    ref: refs.small,
  },
  /* snip - rest is the same as above - snip */
}));
const expectedResult = {
  root: `tss-react-[hash]-RefsExample-root`,
  smallSize: `tss-react-[hash]-RefsExample-smallSize tss-react-ref-RefsExample-small-0`,
  header: `tss-react-[hash]-RefsExample-header`,
  container: `tss-react-[hash]-RefsExample-container`,
};

Regarding the mentioned bug from above with refs

Imho there is an issue in the current implementation with refs, because the counter which is used to generate starts from 0 on each useStyles invocation so the class names it generates are reused multiple times within the application.
I don't think I need to explain any more why this is really bad 😅

Conclusion

Pros

• Better for debugging with name option
• Greatly simplified ref usage which does not require complex inner functions to use it 👍

Const

• Requires breaking API change for refs

I think the above API for makeStyles is way simpler to use overall and would be a great addition worth the breaking change with refs (which are not used that much yet anyway I guess?).
At least I will definitely be using this modified version for makeStyles in my own project until we reach a conclusion and PR state here.
If you think the approach I suggested is fine, please tell me and I could create a PR.

EDIT: One more suggestion: Maybe it would make sense to include the leading . in the refs object so you could write [`& ${refs.myRef}`]: { } instead of [`& .${refs.myRef}]: { } because it gets really easy to forget that extra dot and as it is always necessary, it could be included in the passed ref.

PS: If you have some discord or other means of contact and would like to get into a bit more of a discussion & brainstorming, please tell me 😉

[garronej]

Usage with component function for name (not recommended imho!)

function MyComponent(props) { 
  return <div {...props} />;
}
const useStyles = makeStyles({ name: MyComponent })({
  root: { /* ... */ },
});

We can't rely on Function.prototype.name because all react component should use memo. The component has to be wrapped.

the counter which is used to generate starts from 0 on each useStyles invocation so the class names it generates are reused multiple times within the application.

It's not a bug. Those refs are for selecting children. It's like if you where saying it's a bug to have multiple x variables declared across your application. If they are declared in different scopes no need to explain why it's all good. 😉

Regarding your proposal. It's not bad at all. I'll admit, I was a bit skeptical but your approach is quite clever.
However, I would need you to work a little bit more on the types.

I would need this to pass (or, per say, successfully fail):

import { assert } from "tsafe/assert";
import type { Equals } from "tsafe";

//@ts-expect-error: "xxx" should be added to the record of CSSObject
const useStyles = makeStyles({ "refs": [ "xxx" ] })(
	(theme, props, refs) => ({
		"root": {
			"backgroundColor": "red"
		}
	})
);

const { classes } = useStyles();

assert<Equals<typeof classes, Record<"root", "xxx", string>>();

Otherwise we could end up with error that goes unnoticed like:

const useStyles = makeStyles({ "refs": [ "xxX" ] })(
	(theme, props, refs) => ({
                "xxx": {},
		"root": {
			[`& .${ref.xxX}`]: {
                             //This will never apply because "xxx" !== "xxX"
                        }
		}
	})
);

Also if a class key get rename maintainers will have to remember to rename also the refs.

which are not used that much yet anyway I guess?

Unfortunately, they are already widely used, it's a feature that have been requested.
I regret that we didn't go through this PR before I bumped to v1.0 but if it's really good I'll merge.

[garronej]

You can reach me on this discord server but it's always better to document our exchange here.

[olee]

Otherwise we could end up with error that goes unnoticed like:

That was exactly one of my concerns I noticed while continuing with the mui 5 upgrade in our project as well:
Renaming a class which was used as ref with implicit assignment could introduce errors which get unnoticed.
First I thought of it as a feature to allow users to give the ref a different name than the class, however I think this is actually not really useful and could just introduce bugs.

I will soon create a revised version with the required improvements.

As for the bug: look here
You will see that hovering Root 1 will also trigger the style change for Child 2 except it shouldn't.
If you however uncomment the createRef() in the second makeStyles to force it receiving a different name, it will work correctly.
It's a really nasty tricky bug, but if you think it through for a while you will see what I mean and why this can be an issue.

[olee]

Actually the pattern can easily be very common.
You just need some kind of container component which uses a ref plus some component down the tree which uses one as well.
That's enough to trigger this issue.

[waynebloss]

Is this similar to classNamePrefix from JSS?

import { makeStyles } from "@material-ui/core";

const useStyles = makeStyles(
  theme => ({
    root: { padding: 8 }
  }),
  { classNamePrefix: "ShowAppraisalPage" },
);

[garronej]

@waynebloss something like it yes except that it wont be a prefix but something inserted in the class name.
The prefix is defined by the emotion cache in use. By default it is "tss-react" but you can change it by providing a custom context

[garronej]

Hi @olee,
Do you still plan to make a PR for this?

[garronej]

In 2.0.4 ( the labelling part)