material-ui
material-ui copied to clipboard
[system] Support transformed sx values to allow smoother transition to zero-runtime
Adding support for transformed sx values may have been done if this is used in a zero-runtime setup. I would consider the solution a bit hacky and I am not sure how we would ensure that this will always take presedence.
TODO:
- [ ] Apply type changes after the PR is approved (the change was reverted so that the review is easier)
- [ ] Joy UI components need to explicitly propagate the ownerState, not sure how this worked before 😕
- [ ] Figure out the StepContent failing tests
For reviewers:
- All docs related changes are related to: spreading sx prop needs to happen only if it is an object (adding a string type was failing this) - you cak safely skip this
Netlify deploy preview
https://deploy-preview-40127--material-ui.netlify.app/
@material-ui/core: parsed: +0.21% , gzip: +0.28% @material-ui/lab: parsed: +0.31% , gzip: +0.34% @material-ui/system: parsed: +1.11% , gzip: +1.09% @mui/joy: parsed: +0.66% , gzip: +0.49%
Bundle size report
Details of bundle changes (Toolpad) Details of bundle changes
Generated by :no_entry_sign: dangerJS against c648bc931adff624fd838d36c707e1db4b6c20f3
No major comments towards the implementation. All looks good. I am just wondering if we should test this extensively through the docs site before merging.
I wonder if there is a way that we don't have to create a Tag component within the createStyled again from Emotion. My concern is the performance since isValidProp() is done twice for every styled-components (one from Emotion, one from the createdStyled).
Is it possible to update the Pigment sx transformation to replace it with className and style directly? Pigment exports a runtime sx() that does the same as internal styled.
// input
function App2(props) {
return (
<SliderRail
sx={
props.variant === 'secondary'
? { color: props.isRed ? 'red' : 'blue' }
: { backgroundColor: 'blue', color: 'white' }
}
/>
);
}
// output
import sx from '@pigment-css/react/sx';
function App2(props) {
return (
<SliderRail
{...sx(
props.variant === 'secondary'
? {
className: 's1xbsywq',
vars: {
's1xbsywq-0': [props.isRed ? 'red' : 'blue', false],
},
}
: 's1wnk6s5',
props,
)}
/>
);
}
The implementation of runtime sx() will combine the props and return className and style to the styled-component. With this, we don't have to change any single line of code from the Emotion side.
It's like writing:
function App2(props) {
return (
<SliderRail
className={sx(…).className}
style={sx(…).style}
/>
);
}
cc @brijeshb42
No major comments towards the implementation. All looks good. I am just wondering if we should test this extensively through the docs site before merging.
For sure. I am still not 100% sure about the changes in createStyled there may be subtle changes that could break things in terms of props forwarding. I would still consider these changes a hacky solution to the problem, I think we should seriously consider what are the other options:
- detecting if the changes should be transformed
- use different prop - the migration cost may be higher
- etc.
@siriwatknp we could try this, it may be problematic when there is spreading of props tough
<Component
sx={...}
{...other}
/>
Would need to somehow be converted to:
<Component
{...other}
className={clsx(sx.className, other.className)}
style={{...sx.style, ...other.style}}
/>
Agree performance is the other main reason why I was hesitant to merge this originally (even if we consider it wouldn't introduce breaking changes, while later turned out it will).
we could try this, it may be problematic when there is spreading of props tough
I think it could handled before the transformation because we had access to AST at that time. Just look for the className and style props first, then spread props. It should work.
On the Pigment side, we can remove the logic because the sx runtime already handles it.