Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

with regards to emotion vs jss for mui v5 #28463

Closed
smac89 opened this issue Sep 19, 2021 · 11 comments
Closed

with regards to emotion vs jss for mui v5 #28463

smac89 opened this issue Sep 19, 2021 · 11 comments
Labels
discussion package: system Specific to @mui/system

Comments

@smac89
Copy link

smac89 commented Sep 19, 2021

(Btw I was hoping to post this in the discussions section, but github won't allow me)

Ever since v5 was released, I've noticed a strong push towards using emotion styling (styled) over the previous recommendation of jss (makeStyles).

This weekend I began the process of attempting to migrate our codebase to mui v5. In the process, I decided to try my hands at the new use of styled components. At the end of this little experiment, I am highly doubtful that using emotion style is the way to go for non-library authors.

Firstly, I have very little experience using emotion, and much less styled, but after running the codemods and playing around with the syntax, I came to the following conclusions:

  1. The end result of using styled (especially for large components which have lots of styling), makes the component look more complex than it should be. The reason for this is mostly due to the insistence of having to "wrap" a component with a given style, which then forces us to create a special "StyledXYZ" version of the topmost element, outside the component tree. Then inject it once again into component tree. 😬 yikes! Not to talk about the namespacing with having to prefix all styles with the name of the component, then these have to be interpolated inside template strings...
  2. Styled components become too tightly coupled to the code used to style them. If I decide to change the outermost element of my component to ZXY, I now have to rename my "StyledXYZ" component to "StyledZXY", to maintain the new naming convention.
  3. Due to the two above reasons, I come to a third conclusion that emotion styling seems more suited for library authors who are trying to create a branding around their components, rather than users who are just styling a few parts of their components..
  4. Styled components leak styling props to the component which they wrap. This adds an unnecessary burden on the author to write a filter function (shouldForwardProp) to determine if some props should be forwarded or not. I mean why is that even a question.

At the end of the day for me, this is more readable....

with jss styling
const useStyles = makeStyles<Theme, { mini: boolean, colour?: string }>(theme => ({
    avatar: ({ colour = lighten(theme.palette.secondary.main, 0.7), mini }) => ({
        width: theme.spacing(mini ? 6 : 9),
        height: theme.spacing(mini ? 6 : 9),
        borderColor: colour,
    }),
    avatarText: ({ mini }) => ({
        ...,
    }),
    name: ({ mini }) => ({
        ...,
    }),
    value: ({ mini }) => ({
        ...,
    }),
}));

interface SomeComponentProps {
    colour?: string;
    size?: "mini" | "normal";
}

const SomeComponent: React.FC<SomeComponentProps> = React.memo(({ colour, size = "normal" }) => {
  const classes = useStyles({ mini: size === 'mini', colour: color });
  return (
    <Box  ... >
                ...
    </Box>
  );
}

export default SomeComponent;

than this

with emotion styling
const PREFIX = "SomeComponent";

const classes = {
    avatar: `${PREFIX}-avatar`,
    avatarText: `${PREFIX}-avatarText`,
    name: `${PREFIX}-name`,
    value: `${PREFIX}-value`,
};

const StyledBox = styled(Box, {
    shouldForwardProp(prop) {
        return (typeof prop !== 'string' || prop[0] !== '$');
    }
})<{ $mini: boolean, $colour?: string }>(({ theme, $mini, $colour = lighten(theme.palette.secondary.main, 0.7) }) => ({
    [`& .${classes.avatar}`]: {
        width: theme.spacing($mini ? 6 : 9),
        height: theme.spacing($mini ? 6 : 9),
        borderColor: $colour,
        ...,
    },
    [`& .${classes.avatarText}`]: {
        ...,
    },
    [`& .${classes.name}`]: {
        ...,
    },
    [`& .${classes.value}`]: {
        ...,
    },
}));

interface SomeComponentProps {
    colour?: string;
    size?: "mini" | "normal";
}

const SomeComponent: React.FC<SomeComponentProps> = React.memo(({ colour, size = "normal" }) => {
  return (
    <StyledBox $mini $colour={colour} ...>
                ...
    </StyledBox>
  );
}

export default SomeComponent;

What are your thoughts on this? Apart from the promise of decreased bundle size, what other real advantage does emotion offer over jss?

@smac89 smac89 added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Sep 19, 2021
@Talendar
Copy link

I really like JSS. I don't understand the appeal of emotion/styled-components.

@mnajdova
Copy link
Member

@smac89 would have been great if this would have been posted as a comment into #22342 to follow the discussions that we already had on the topic.

I will try to briefly provide some information on the mater. We offer two APIs for styling/customizing:

  • styled() API (which you described above), which is supposed to be used for reusable components
  • sx prop, which should be used for one-off customizations.

The end result of using styled (especially for large components which have lots of styling), makes the component look more complex than it should be. The reason for this is mostly due to the insistence of having to "wrap" a component with a given style, which then forces us to create a special "StyledXYZ" version of the topmost element, outside the component tree. Then inject it once again into component tree. 😬 yikes! Not to talk about the namespacing with having to prefix all styles with the name of the component, then these have to be interpolated inside template strings...

Are you sure in this case you shouldn't have just use sx on the elements?

Styled components become too tightly coupled to the code used to style them. If I decide to change the outermost element of my component to ZXY, I now have to rename my "StyledXYZ" component to "StyledZXY", to maintain the new naming convention.

Note that the automatic conversion of the styles we have with the codemod is not ideal and should not be understood as a recommended way of styling (it is suppose to help you remove the dependency on JSS sooner rather than later).

I would recommend going over the:

These should give you an idea between the different ways of customization and which ones should be used when. If you still want to use the makeStyles API you can by:

@mnajdova mnajdova added discussion package: system Specific to @mui/system and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Sep 20, 2021
@garronej
Copy link
Contributor

garronej commented Sep 20, 2021

Hi,
If you chose to go with tss-react you will end up with something
like that:

const useStyles = makeStyles<{ mini: boolean, color?: string }>()(
	(theme, { color = lighten(theme.palette.secondary.main, 0.7), mini }) => ({
		avatar: {
			width: theme.spacing(mini ? 6 : 9),
			height: theme.spacing(mini ? 6 : 9),
			borderColor: color
		},
		avatarText: {
			...
		},
		name: {
			...
		},
		value: {
			...
		}
	})
);

interface SomeComponentProps {
	color?: string;
	size?: "mini" | "normal";
}

const SomeComponent: React.FC<SomeComponentProps> = React.memo(({ color, size = "normal" }) => {
	const { classes } = useStyles({ mini: size === 'mini', color });
	return (
		<Box  ... >
                ...
    </Box >
  );
}

export default SomeComponent;

Cool thing is your class object will be of type:

{
    avatar: string;
    avatarText: string;
    name: string;
    value: string;
}

it's much more typesafe than just Record<string, string>.
Other important thing to note is that tss-react is implemented on top of @emotion/react so there is virtually no impact on the bundle size when used alongside mui v5.

@smac89
Copy link
Author

smac89 commented Sep 20, 2021

@mnajdova thank you for the clarification. I must have missed tss-react when combing the docs last weekend.

@garronej I have just briefly went over the docs on tss-react and I will certainly take this route seeing as it provides the least amount of work for adopting mui v5.

oh and this line right here:

(theme, { color = lighten(theme.palette.secondary.main, 0.7), mini }) => ({

👌 absolute clutch move.
This was the only thing I missed as I reverted all changes made by the codemod, but looks like I don't have to regret for too long.

Thanks all!

@aleccaputo
Copy link
Contributor

@garronej @smac89
It would be easy for us to just use tss-react (as we are also a large org that appreciated the jss api). My hesitation comes from the MUI upgrade docs doesn't give me a lot of confidence in choosing it. It's maintained outside the MUI org, and i'm nervous that if i use it i'll just be shooting myself in the foot later.
Am i going to regret this decision when MUI 6 releases? I'm just unsure.

@garronej
Copy link
Contributor

Hi @aleccaputo,
What I can say is that I am committed to maintaining tss-react for the foreseeable future.
I take great care to ensure that tss-react do integrate well with MUI and I always respond to issues.

@smac89
Copy link
Author

smac89 commented Sep 20, 2021

@aleccaputo yea let's hear what the maintainers have to say about this. Although the docs mention that tss-react is not maintained by the mui team, it is still mentioned as an alternative. My confidence in going with tss-react is two-fold:

  1. It provides the path of least resistance when transitioning from jss to emotion. As the docs say:

    'tss-react' is intended to be a replacement for 'react-jss'

    ...so right off the bat, we have knocked off the dependency on jss just as the mui 5 has done, and we have also transitioned to emotion just as mui 5 has done.

  2. Even with this transition, we have not lost the syntax we were used to in mui 4 with jss. The syntax remains largely the same, so any code refactoring wouldn't be a nightmare to review.

I should add that you may want to carefully read the docs on tss-react because it doesn't fully support all features of react jss namely, "nested rule selectors". The workaround provided is not entirely frictionless, but still not a deal breaker (for me).

@aleccaputo
Copy link
Contributor

aleccaputo commented Sep 24, 2021

@garronej @smac89
Good points. Considering the lift to switch to tss-react is MUCH less than to styled/sx, might as well give it a shot. Worst case is if something does go wrong, i'd basically just be in the same position i'm in now anyways (but a little better since i'd be using emotion and will have removed @mui/styles).

@aleccaputo
Copy link
Contributor

aleccaputo commented Sep 24, 2021

@garronej does <CacheProvider /> replace <ThemeProvider /> from mui? And therefore do i no longer need createTheme?

@garronej
Copy link
Contributor

Hi @aleccaputo,
No, it doesn't, you still need <ThemeProvider >. <CacheProvider /> replaces <StylesProvider injectFirst />.

@mnajdova
Copy link
Member

I am closing the issue, it's been a while and it seems like most of the questions it opened are answered.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion package: system Specific to @mui/system
Projects
None yet
Development

No branches or pull requests

5 participants