material-ui icon indicating copy to clipboard operation
material-ui copied to clipboard

[system] Support transformed sx values to allow smoother transition to zero-runtime

Open mnajdova opened this issue 2 years ago • 6 comments

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

mnajdova avatar Dec 06 '23 15:12 mnajdova

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

mui-bot avatar Dec 06 '23 15:12 mui-bot

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.

brijeshb42 avatar Apr 08 '24 09:04 brijeshb42

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

siriwatknp avatar Apr 08 '24 09:04 siriwatknp

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.

mnajdova avatar Apr 08 '24 09:04 mnajdova

@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).

mnajdova avatar Apr 08 '24 09:04 mnajdova

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.

siriwatknp avatar Apr 08 '24 09:04 siriwatknp