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

[material-ui] Avoid ownerState silent propagation #42184

Closed
DiegoAndai opened this issue May 9, 2024 · 1 comment · Fixed by #42358
Closed

[material-ui] Avoid ownerState silent propagation #42184

DiegoAndai opened this issue May 9, 2024 · 1 comment · Fixed by #42358
Assignees
Labels
package: material-ui Specific to @mui/material

Comments

@DiegoAndai
Copy link
Member

DiegoAndai commented May 9, 2024

Following-up on #42062 (comment)

Problem

With the standardization of the slots pattern, we've run into this multiple times:

  • Parent component has ownerState, let's call it ownerStateParent
  • Child slot component has it's own ownerState, let's call it ownerStateChild, that it needs to provide to its own children

The ownerStateParent is provided to the child slot component, as it's useful when overriding.

But the child needs to take care of two things:

  • That the ownerStateParent, coming through props, doesn't override its own ownerStateChild when passing it to its own children. This is the reason for the change on lines 118-123: ownerState={ownerState} must come after {...other}.
  • That the ownerStateParent is not nested inside its own ownerStateChild, otherwise, it would override ownerStateChild because of how createStyled merges props (reference). This could cause bugs when accessing the ownerState in style variants props matching. Like it's done in this component (line 68). This is the reason for delete ownerState.ownerState on this file.

The pattern of spreading other into the root component, as well as spreading props into ownerState, is spread throughout the codebase, and that's why we've been bumping into this repeatedly. This behavior is error-prone, as it's not evident unless you're familiar with the codebase.

Ideas

  • It might be worth it to invest in a shared mechanism that all components can rely on to safely spread other and/or props without propagating unwanted ownerStates.
  • Never inherit other component which turns out to be easier to maintain. Always inherit styled-component (to get styles) from another component instead of wrapping the whole component. (reference)
  • Abstract this inside the useSlot hook. The problem with this one is that most components would need to use useSlot.

Search keywords:

@lhilgert9
Copy link
Contributor

@DiegoAndai From my point of view, another idea could be to delete props.ownerState in the useThemeProps hook or in every component before the props are spreaded. This should solve the problem with ownerState.ownerState and also that the parentOwnerState appears in ...other or ...props. I find it difficult to handle the whole thing in the useSlot hook, as the ownerState would no longer be passed to styled components if I'm not wrong.

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

Successfully merging a pull request may close this issue.

2 participants