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

[POC] ownerState access: use context #35654

Closed

Conversation

michaldudak
Copy link
Member

@michaldudak michaldudak commented Dec 28, 2022

DO NOT MERGE

This is a playground that illustrates an alternative way of accessing ownerState in slot components.
A SwitchUnstyled is used for this example.

Related issue: #32882

This is one of the possible solutions. The other one is described in #35668.

A new hook, useSwitchUnstyledOwnerState is introduced. It can be used within slot components to get the state of the owner component and affect what's being rendered.
In this example, a different emoji is displayed depending on the state of the Switch.

Advantages over the existing API:

  1. All props passed to the slot component can be forwarded to its DOM element. This is especially important when using 3rd party components as slots, as they don't know anything about our ownerState prop and may forward everything they receive. ownerState should not be placed on a DOM element as it's not a valid attribute (and React rightfully complains about this).
  2. When using MUI System to style components, developers won't need to configure it to forward ownerState.
  3. Merging ownerStates from different "layers" (i.e., Base and Joy components) won't be an issue anymore. Each ownerState will be in a different context and will be able to be accessed independently. Also this will eliminate the problems with ownerState types we currently have when using Base components as slots in other Base components.

Disadvantages:

  1. The new API is more verbose and harder to find. It may be simpler to type const { ownerState } = props; than const ownerState = useSwitchUnstyledOwnerState();. It won't be possible to use a shorthand notation to define a simple component:
    // existing:
    const MyButton = ({ ownerState, ...other }) => <button {...other} className={ownerState.active ? 'active' : ''} />;
    
    // new:
    const MyButton = (props) => {
      const { active } = useButtonUnstyledOwnerState();
      return <button {...props} className={active ? 'active' : ''} />;
    };
  2. Extensive use of contexts may negatively affect performance (I haven't done any measurements yet, though)

Playgrounds

New API: https://codesandbox.io/s/sharp-booth-hx3xtn?file=/src/App.tsx

Existing API: https://codesandbox.io/s/confident-hermann-q1kiwf?file=/src/App.tsx

@michaldudak michaldudak added the proof of concept Studying and/or experimenting with a to be validated approach label Dec 28, 2022
@mui-bot
Copy link

mui-bot commented Dec 28, 2022

Messages
📖 Netlify deploy preview: https://deploy-preview-35654--material-ui.netlify.app/

@material-ui/unstyled: parsed: +0.34% , gzip: +0.37%

Details of bundle changes

Generated by 🚫 dangerJS against cfb8af2

@mnajdova
Copy link
Member

I like this direction, the code looks much cleaner. Few thoughts:

  • I think we should rename the hook to simply useSwitchState - this is exactly what it is. Now that the hook has the name of the component, it's not needed anymore to include the ownerPrefix.

  • Merging ownerStates from different "layers" (i.e., Base and Joy components) won't be an issue anymore. Each ownerState will be in a different context and will be able to be accessed independently. Also this will eliminate the problems with ownerState types we currently have when using Base components as slots in other Base components.

    This sounds great!

  • Extensive use of contexts may negatively affect performance (I haven't done any measurements yet, though)

    Maybe we can render in the benchmarks 1000 Switches with the previous implementation and the new one for comparison

Question:

  • What would this mean for the Joy UI components? How big of a change will this be (I suppose the changes are repetitive, but just to consider it).

@michaldudak
Copy link
Member Author

michaldudak commented Dec 28, 2022

I think we should rename the hook to simply useSwitchState - this is exactly what it is

Good point. But I think useSwitchUnstyledState could be better as Material UI and Joy may have their own versions of this hook since they also allow overriding slots and their ownerState may be richer than the Base's.

What would this mean for the Joy UI components? How big of a change will this be

In a perfect world, neither Joy nor Material UI would notice this change as it affects only the unstyled components, not hooks. But AFAIR we still have a couple of Joy components implemented based on components, so some changes will be necessary.


One thing I realized is that it may not be necessary to include all of the owner props in the ownerState - just the calculated internal state, such as checked (+ perhaps some of the props for convenience, such as disabled). If we include all the props, the context value will change each render, which will hurt performance for sure.

@mnajdova
Copy link
Member

Good point. But I think useSwitchUnstyledState could be better as Material UI and Joy may have their own versions of this hook since they also allow overriding slots and their ownerState may be richer than the Base's.

Sounds good 👍

One thing I realized is that it may not be necessary to include all of the owner props in the ownerState - just the calculated internal state, such as checked (+ perhaps some of the props for convenience, such as disabled). If we include all the props, the context value will change each render, which will hurt performance for sure.

Yes, we should be mindful of what's in here and make sure to memoize it.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 1, 2023

One interesting feature of this direction is that it allows the decoupling of the rendering of the slots from the parent. The slots can be rendered by userland.

My main question is how will slots style themselves if styled() doesn't no longer have access to the ownerState?

If we include all the props, the context value will change each render, which will hurt performance for sure.

Should we care about this? From my perspective, since these are leaf components, they are meant to rerender. I would expect a memorization to waste CPU cycles overall.

@michaldudak
Copy link
Member Author

My main question is how will slots style themselves if styled() doesn't no longer have access to the ownerState?

We still apply CSS classes that can be used for styling.

Should we care about this? From my perspective, since these are leaf components, they are meant to rerender. I would expect a memorization to waste CPU cycles overall.

They aren't always leaf components. Compound components, such as Select render deeper trees inside slots, so in some cases preventing rerendering when unnecessary may save many CPU cycles. But for simple components you're right - there may be no gains.

@michaldudak michaldudak added the package: base-ui Specific to @mui/base label Jan 3, 2023
@michaldudak michaldudak changed the title [POC] Use context to get ownerState in slots [POC] ownerState access: use context Jan 3, 2023
@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 9, 2023

We still apply CSS classes that can be used for styling.

@michaldudak There are some, but not all the cases are covered by the class names. Say I want to change the background color on a lower element based on the color prop of the root. How to do this?

They aren't always leaf components. Compound components, such as Select render deeper trees inside slots, so in some cases preventing rerendering when unnecessary may save many CPU cycles. But for simple components you're right - there may be no gains.

👍 to explore, I would be amazed if there are cases where it makes us one step ahead and not behind.

@michaldudak
Copy link
Member Author

michaldudak commented Jan 11, 2023

Say I want to change the background color on a lower element based on the color prop of the root. How to do this?

I'd expect the color prop would set a class name.

But in general, yes, your point is valid - there could be cases where we don't expose everything as class names.

One idea that just occurred to me is to use higher-order components when you want to use ownerState inside styled:

const StyledThumb = styled(WithSwitchState('span'))({ ownerState } => ` ... `);

I'll prepare another playground showing this option.

EDIT: this doesn't seem to be possible.

One way you could do this is:

const Track = styled('span')(() => {
  const { checked } = useSwitchUnstyledOwnerState();
  return `
    outline: 3px solid ${checked ? 'purple' : 'grey'};
  `;
});

But it makes the style definition longer.

@mnajdova
Copy link
Member

Based on the use-cases we tried so far, this is what I could summirize:

  1. For using the styled components styling paradigm the ownerState as a prop works better. Why? It doesn't require creating custom component for propagating the state from the unstyled component.
  2. For using CSS, SASS etc. in my opinion the ownerState as prop still works better, as MUI Base will not propagate it to the DOM nodes, but it will be available in the slotProps for applying custom class names. If developers don't feel comfortable with this API they can still create custom components and do the map of ownerState to class names/props in the custom components. If they choose this way for customization, they may as well use the useComponentUnstyledState hook if available.
  3. Third party components - using the useComponentUnstyledState would work better, as they wouldn't have by default ownerState prop propagated to the component. How likely is that third party component won't require any custom logic wrapped around them it's not clear to me - it may be the case with the link components, but apart from that I am not sure.

So it boils down to what we want to optimize for. We shouldn't put all bet in styled components, because as the server components are going to become stable, I don't see a way for big growth, I feel like people will progressivly move to either static extraction styling soltuion, or different utility first classes frameworks, like Tailwind CSS.

My ultimate question would be, could we support both and create guides that will teach developers what to so use based on their styling solution (maybe we can have a global provider for configuring the component about which paradigm to use (context vs prop propagation)). Then we can add a guide for how to use MUI Base with styled-components, utility first classes, plan CSS etc.

@michaldudak
Copy link
Member Author

Using CSS, SASS, Tailwind, and similar (basically anything other than styled components-like solutions) doesn't require using the slots API. slotProps should be more straightforward in this case.

IMO the use cases for the slots API are:

  1. Styled components - I agree, ownerState as a prop is easier to use
  2. Third party components - useOwnerState clearly wins in this case, as the external components would work without any wrapping.
  3. Custom 1st party components, such as the one shown in the linked sandbox. It's not apparent which solution is better in this case. A prop makes it easier to access ownerState (no need to import an additional hook). However, the ownerState prop must be manually excluded from forwarded props, so it doesn't appear on the DOM.

One other advantage of a hook is simplified (and correct) types. If we had ownerState as a prop, which presence is controlled by another prop on an owner, it would be close to impossible to define a correct type for the slot component props. We would have to fall back to ownerState?: OwnerStateType and developers would have to perform null checks or assert a non-null field with !.
With ownerState exposed as a hook, this problem doesn't exist at all.

@mnajdova
Copy link
Member

  1. Styled components - I agree, ownerState as a prop is easier to use

Nice, we are on the same page 👍

  1. Third party components - useOwnerState clearly wins in this case, as the external components would work without any wrapping.

Agree 👍

  1. Custom 1st party components, such as the one shown in the linked sandbox. It's not apparent which solution is better in this case. A prop makes it easier to access ownerState (no need to import an additional hook). However, the ownerState prop must be manually excluded from forwarded props, so it doesn't appear on the DOM.

Agree, I would say it would be a personal preference.

So, if we weight these three groups, the third party components are the least usage (a guess would be 5%, maybe even less, correct me if you feel like this is wrong as it is truly just a guess). Considering that for 3. it's not clear what API is better, I would say maybe using the hook is a bit more the "React way of doing things" I would weight it a bit more than the ownerState props. Even with this, looks like the ownerState as prop is the best alternative for most of the cases, which is why I would propose using it by default. The question is, should we maybe allow people to use context conditionally? How much of an effort would this be? Can we minimize the runtime so that we don't have unnecessary calculations for the different ways of providing the ownerState? This would allow us to collect more feedback. In the usage page we can add guides for different use-cases as I mentioned above.

@michaldudak
Copy link
Member Author

👍 The remaining issue is the default value.

  1. If we don't provide the ownerState among props by convention, the default behavior will not cause errors if someone forwards all the props to the DOM element. If someone needs the ownerState, they'll have to explicitly enable it.
  2. If we enable it by default, styling the components using ownerState will immediately be possible, but at the cost of throwing warnings when a 3rd party component is used. It will essentially be the existing behavior with the possibility of configuring it.

@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 13, 2023

So, if we weight these three groups, the third party components are the least usage (a guess would be 5%, maybe even less, correct me if you feel like this is wrong as it is truly just a guess).

I think that the Link component use case is very common (almost all developers will face it). From the perspective of a developer using the design system built with MUI Base, I think that it's a lot more than 5%. From the perspective of a developer that creates a design system with MUI Base, I agree.

Here https://mui.com/material-ui/guides/routing/#global-theme-link we require to create a wrapper but it's only for global use with the theme.

maybe using the hook is a bit more the "React way of doing things"

As I understood hooks don't work with server components because the context doesn't: https://twitter.com/sebmarkbage/status/1587615484870098945.

I feel like people will progressivly move to either static extraction styling soltuion, or different utility first classes frameworks, like Tailwind CSS.

With static extraction, Vanilla Extract API looks like this: https://github.com/timoclsn/mauli-vanilla-extract/blob/main/src/components/Button/Button.tsx#L49. Their API operates at the className level, so I guess same case as "CSS, SASS etc".


Since https://mui.com/material-ui/guides/routing/#link seems to work ok, It makes me wonder, how do we already solve this problem with Material UI? What would it take to reproduce the solution?

@michaldudak
Copy link
Member Author

As I understood hooks don't work with server components because the context doesn't

Is it a problem, though? All of our unstyled components are client ones, as they are interactive, use state, effects, etc. (see the RFC for limitations of server components).

Since https://mui.com/material-ui/guides/routing/#link seems to work ok, It makes me wonder, how do we already solve this problem with Material UI? What would it take to reproduce the solution?

We don't expose the ownerState in Material UI components.

@oliviertassinari
Copy link
Member

All of our unstyled components are client ones

My thought was that maybe they don't all need to be. For some of them, it can make sense to be server-side only: an input element, in the footer, for a newsletter subscription can be server-side only. The same for the button that comes with it. A badge can be visual only too. But yeah, it won't make a huge difference at the end of the day 😁

@michaldudak
Copy link
Member Author

My thought was that maybe they don't all need to be. For some of them, it can make sense to be server-side only: an input element, in the footer, for a newsletter subscription can be server-side only

This would require rewriting our components not to use effects or state, stripping them out of functionality. And if we take out functionality of an unstyled component, I don't think there will be any value left.

I understand the value of server components is to provide data to the client, as they are closer to other servers and databases. Right now, I can't see the value of having UI controls as server components.

@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 15, 2023

@michaldudak OK, fair enough.

This would require rewriting our components not to use effects or state, stripping them out of functionality. And if we take out functionality of an unstyled component, I don't think there will be any value left.

We could make useEffect a no-op and useState to return the default value.

I understand the value of server components is to provide data to the client, as they are closer to other servers and databases. Right now, I can't see the value of having UI controls as server components.

I was considering the case where a design system is built with MUI Base, and mostly uses it for client-side, but where the developers still have a few basic use cases for them for server components. Effectively relying on the default behavior of an <input> and a <button>.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: base-ui Specific to @mui/base PR: out-of-date The pull request has merge conflicts and can't be merged proof of concept Studying and/or experimenting with a to be validated approach
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants