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

[codemod] Skip sx spread transformation #43291

Merged
merged 2 commits into from
Aug 14, 2024

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented Aug 14, 2024

closes #43230

<FormControl
  disabled={disabled}
  id={id}
  sx={[...(Array.isArray(formControlSx) ? formControlSx : [formControlSx])]}
/>;

The code above is a type of sx spread which works with MUI System. The codemod should not transform this code.

Unrelated to this PR but need to check if Pigment CSS will skip this code or not.


@mui-bot
Copy link

mui-bot commented Aug 14, 2024

Netlify deploy preview

https://deploy-preview-43291--material-ui.netlify.app/

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 323278f

@siriwatknp siriwatknp requested review from mnajdova and LukasTy August 14, 2024 04:32
@siriwatknp siriwatknp added bug 🐛 Something doesn't work package: codemod Specific to @mui/codemod v6.x and removed bug 🐛 Something doesn't work labels Aug 14, 2024
Comment on lines +343 to +346
(data.node.argument.test.type === 'CallExpression' &&
data.node.argument.test.callee.type === 'MemberExpression' &&
data.node.argument.test.callee.object.name === 'Array' &&
data.node.argument.test.callee.property.name === 'isArray') ||
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure that we don't need the sx prop name check in this rule as well? 🤔
I suspect that given the existing rule, it would skip the following prop as well, even though it probably shouldn't. 🤔

xyz={[
  ...(Array.isArray(xyz) ? xs : [xyz]),
  ...(Array.isArray(slotProps?.layout?.xyz) ? slotProps.layout.xyz : [slotProps.layout.sxxyz),

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This codemod only targets sx prop, so it will not look into this xyz prop.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I saw sx checks in the || else rules and assumed these are the only checks. 🙈
LGTM then, sorry for the distraction. 👍

Copy link
Member

@LukasTy LukasTy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking care of it. 🙏

Comment on lines +343 to +346
(data.node.argument.test.type === 'CallExpression' &&
data.node.argument.test.callee.type === 'MemberExpression' &&
data.node.argument.test.callee.object.name === 'Array' &&
data.node.argument.test.callee.property.name === 'isArray') ||
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I saw sx checks in the || else rules and assumed these are the only checks. 🙈
LGTM then, sorry for the distraction. 👍

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

Successfully merging this pull request may close these issues.

[codemod] v6.0.0/sx-prop produces strange changes
3 participants