-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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
[RFC] Add makeStyles back? #26571
Comments
As far as I know, @layershifter was also working on a similar utility for Fluent-UI, tagging him to get his opinion on the matter. My biggest worry is that it would be the first utility that would be emotion's only API, which will break our story of supporting two styling engines. Not sure how big the cost would be for building and supporting the API. On the other hand, I understand the pain of the migration, and even more importantly I understand why developers may prefer this API. Things we need to consider:
|
Thanks for considering this. I have almost forced to cancel plan of v5 migration on our app coz there are loads of effort involved to replace makeStyles with styled or sx. |
@thecodejack option 1 is not the only option, 2 and 3 are also valid. You could wait v6 to migrate the |
Add |
I am also hugely in favor of adding back makeStyles. My team and I were worried that we would be permanently locked into v4 due to the lack of makeStyles support - we've created an entire component library wrapping material components, and all of it uses the makeStyles hook. It would be a massive bonus if this update/migration to makeStyles also fixed the known performance issues of passing props into makeStyles - it's something we've been trying to avoid, but on a large team, knowledge share is hard and people keep doing it. As for reasons that we prefer makeStyles:
We don't have anywhere near enough bandwidth to migrate to StyledComponents or emotion, it would be at least 500 components, likely many more between our component library and all the apps that use them. We might be able to migrate to react-jss, which we were considering, but there was some slight inconsistencies in the behavior of makeStyles vs createStyles which concerned me, not to mention the bundle size increase of including react-jss and emotion/styled for mui. |
Yes, we even are blocked on |
To upgrade to v5, we need to choose between https://github.com/garronej/tss-react (already available today) and this RFC (not implemented yet). Hope the team can decide this topic early (implement makeStyles in emotion, or encourage people to migrate to tss-react) so we don't have to bet on choosing either solution. Thanks! |
I am open to work with you on this. If you'd like @oliviertassinari, we can discuss what would be the requirements that tss-react should meet to be officially recommended by you guys. |
It's interesting, it's the first time I am seeing it. Let me test out and see what you've got already and I could come back to you next week. |
It’s important to keep makestyles feature there because some projects only use material ui as a component library instead of the css solution. For example, people only use the textfields, checkbox etc, but control the page layout by css-grid instead of Mui grid and control padding using raw css instead of Box. Thus the team would have more flexibility to access the latest css technology instead of been only relying on what mui supports. Also the tss package looks pretty cool and looking forward something could be built based on it. |
@garronej I have open garronej/tss-react#3 so that we can start the discussion :) |
Hope your team will add makeStyles back soon. I had to write my own makeStyles function. 😢
|
Thanks, this is actually helpful. I will implement this approach in tss-react it will enable to rely on Thanks for sharing! |
@garronej I read emotion source code and copied it from this https://github.com/emotion-js/emotion/blob/dcc72d06ace804330fd285a76c8574f3a89001f9/packages/css/src/create-instance.js#L78. |
Hi @mnajdova ,
New GIF: You can give the new version a try in the test apps. Hopes it meets your needs. |
@garronej I was testing the latest changes and could not spot any issues 🚀 I would say that we can recommend in the Migration guide for people that want to still use the Having this said, we are not going to add the Some additional comments regarding the implementation in tss-react (@garronej please correct me if I am wrong somewhere):
-const useStyles = makeStyles(theme => ({
+const { useStyles } = makeStyles()(theme => ({
button: {
color: theme.limeGreen,
}
}));
|
Thank you for the kind words. I am very glad we successfully collaborated on this 😊
You got everything right. I just have a few comments.
I will not re-implement this feature because it can't be implemented in a type safe way but I will implement an other mechanisms for composition
No, it's not, well of course it will work but they will end up with both Let me know if you need anything, Best |
@mnajdova I'm closing then. Thanks to everyone for participating. Since I have opened the issue, I'm going to bring my perspective on why we are not moving forward. In this RFC, I have drawn two value propositions:
For those that prefer working in the makeStyles why the styled and sx API allows them to very similar outcomes. This example: import clsx from 'clsx';
import { makeStyles } from '@material-ui/styles';
const useStyles = makeStyles((theme) => ({
grid: {
display: 'grid',
gridGap: theme.spacing(2),
gridTemplateColumns: '1fr 1fr',
[theme.breakpoints.down('md')]: {
gridTemplateColumns: '1fr',
gridGap: theme.spacing(1),
},
},
column: {
gridTemplateColumns: '1fr',
},
margin: {
margin: theme.spacing(1),
},
}));
const Example = () => {
const classes = useStyles();
return (
<div className={clsx(classes.grid, classes.margin)}>
<div className={clsx(classes.grid, classes.column)}>{/* ... */}</div>
<div className={clsx(classes.grid, classes.column)}>{/* ... */}</div>
</div>
);
}; Can be written with import clsx from 'clsx';
import { styled } from '@material-ui/system';
const classes = { grid: 'grid', margin: 'margin', column: 'column' };
const Root = styled('div')(({ theme }) => ({
[`&, & .${classes.grid}`]: {
display: 'grid',
gridGap: theme.spacing(2),
gridTemplateColumns: '1fr 1fr',
[theme.breakpoints.down('md')]: {
gridTemplateColumns: '1fr',
gridGap: theme.spacing(1),
},
},
[`&, & .${classes.margin}`]: {
margin: theme.spacing(1),
},
[`& .${classes.column}`]: {
gridTemplateColumns: '1fr',
},
}));
const Example = () => {
return (
<Root className={clsx(classes.grid, classes.margin)}>
<div className={clsx(classes.grid, classes.column)}>{/* ... */}</div>
<div className={clsx(classes.grid, classes.column)}>{/* ... */}</div>
</Root>
);
}; @siriwatknp has a WIP codemod to automate this type of migration: #27292. or import clsx from 'clsx';
import { mui } from '@material-ui/system';
const styles = {
grid: {
display: 'grid',
gridGap: {
xs: 2,
md: 1,
},
gridTemplateColumns: {
xs: '1fr',
md: '1fr 1fr',
},
},
margin: {
margin: 1,
},
column: {
gridTemplateColumns: '1fr',
},
};
const Example = () => {
return (
<mui.div sx={{ ...grid, ...margin }}>
<mui.div sx={{ ...grid, ...column }}>{/* ... */}</mui.div>
<mui.div sx={{ ...grid, ...column }}>{/* ... */}</mui.div>
</mui.div>
);
}; |
Your example isn't type checked so if there is a typo, the type system cannot catch it for me. Ant btw will tss-react recommended now? |
|
@oliviertassinari that example doesn't work, and also, isn't the same thing:
No one like breaking changes and I'm sure this was a difficult decision, but I can't help but feel like the migration path and drawbacks have not been considered very thoroughly. |
@sarink thank for pointing out. Here is a working example: https://codesandbox.io/s/kind-resonance-ldo25?file=/src/Demo.tsx
This is for the host elements. The components are used as:
For dynamic element rendered, you can use the
|
@mnajdova what about typesafety? |
Also came here hoping the answer was "we're going to add it back", and disappointed that that's not the case. (Same reasons as everyone else: styled is ugly, verbose and fragments style definitions; losing type safety is a step backwards.) I appreciate the efforts of react-tss but I want something that's really as close to the API of makeStyles of possible, without worrying about access to css, cx etc. So, building on the work of @nguyenvanthanh97, I added in simple dynamic style support. It still doesn't have "$" support, might have bugs, but appears to be type-safe and successfully support all the things I use: import createCache from '@emotion/cache'
import { serializeStyles } from '@emotion/serialize'
import { Interpolation } from '@emotion/styled/types/base'
import { insertStyles } from '@emotion/utils'
import { Theme, useTheme } from '@material-ui/core/styles'
import { useMemo } from 'react'
const createEmotionCache = () => {
return createCache({ key: 'css', prepend: true })
}
const cache = createEmotionCache()
const makeStyles = <Key extends string, Props extends {} = {}>(
generateStyles: (theme: Theme) => Record<Key, Interpolation<Props>>
) => {
return (props?: Props) => {
const theme = useTheme()
const classes = useMemo(() => {
const styles = generateStyles(theme)
return Object.keys(styles).reduce((s, key) => {
const serialized = serializeStyles<Props>([styles[key as Key]], cache.registered, props)
s[key as Key] = `${cache.key}-${serialized.name}`
insertStyles(cache, serialized, false)
return s
}, {} as Record<Key, string>)
}, [theme, props])
return classes
}
}
const createStyles = <Key extends string, Props = {}>(
styles: Record<Key, Interpolation<Props>>
): Record<Key, Interpolation<Props>> => styles
export { createEmotionCache, createStyles, makeStyles } Use it like this:
|
Again, we don’t plan to add the API as part of the library and we are recommending using tss-react in the migration guide for the people that want to use the same API. Regarding tss-react, we’ve also tested server side rendering to ensure that it works (I don’t think it works with the codesnipet you shared). It is type safe as well, as last when I saw there was a PR for adding an API as a replacement for the $ syntax. @garronej can give more info on this. |
Hi @majelbstoat, -const classes = useStyles();
+const { classes } = useStyles(); As @mnajdova mentioned, in TSS much effort have been put into making SSR work and to feature support of custom emotion cache. If you choses to go with your custom implementation and if you need SSR remember that it won't be trivial to get it working. Regarding the If you have a specific request regarding |
Our project has 100% migrated to tss-react and feeling good.
Could you implement const StyledLinearProgress = withStyles(LinearProgress)({
root: {},
// other LinearProgressProps.classes properties...
}) |
@Jack-Works I opened an issue for |
Yep, no worries, I understand it's not coming back. I think it's a bad call which makes styling in Material UI harder to reason about, but v5 is also a major version jump and y'all considered it carefully, so no hard feelings, and I wasn't trying to re-legislate the decision. I just offered this up for folks who are in the same boat, of which there seem to be a few.
Oh yeah, thanks for the pointing that out. I had to add in
TSS's makeStyles is also invoked differently, and when I dug through the code to understand how it worked, I got lost in a maze of factories. I know these seem like minor quibbles, but for a large legacy codebase it's just easier to have something that's directly compatible and easy to understand. |
@oliviertassinari @mnajdova I get that you're not going to add the old API back in... But is there really no option for typesafe styles? This question keeps getting asked and subsequently avoided. |
@majelbstoat thank you for aknolaging the work! 😊
I get that having exactly the same API makes the migration process seemless but the small API changes that have been made in Regarding TSS react having a more complexe code compared to the snippet you shared, well, it's the price to pay for having a more polished API and a more optimized implementation. |
@sarink Regarding typesafe, I have updated #26571 (comment) to work better. If you want to avoid the boilerplate, https://github.com/garronej/tss-react is a great option. |
|
Summary
Implement a
makeStyles
API back powered by emotion.Basic example
Motivation
We currently plan to remove all support of the
@material-ui/styles
package in v6. This direction creates two main problems.v4 to v5 migration
The migration from makeStyles to the
styled
andsx
API is expensive and very hard to automate. It's unlikely that most projects will have the bandwidth to migrate. This leaves these projects with a limited number of options, none are great.@material-ui/styles
until the end of the rope. They won't get access to bug fixes as they would do in 1. and 2. (react-jss is no very active, so maybe mostly in 1.) Developers will have to load emotion & JSS in their bundle.DX
Based on #16947 (comment), we started to get interesting feedback on why
makeStyles
is great. By order of preference, I would personally rank them: 1. sx 2. makeStyles 3. styled(). The main argument we got was the capability to have multiple class names declared side by side. This constraint yields a couple of interesting properties:classes
object of class names you can forward to any component, portaled element includedDetailed design
There is a proof of concept done by @garronej in https://github.com/garronej/tss-react. However, I'm not sure that we can leverage the approach (
@emotion/css
). We might want to rely on@emotion/react
using the source of ClassName as inspiration.Drawbacks
I can see a couple of classes of drawback:
Alternatives
Do nothing.
Adoption strategy
We don't want it to be significantly adopted, just enough by the teams that have a real need for it.
How we teach this
We implement the same API as in v4, we document the differences regarding global configuration in the migration guide. For instance, they won't be a class name generator anymore.
Unresolved questions
The text was updated successfully, but these errors were encountered: