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

[examples] Fix Pigment CSS Vite example

Open oliviertassinari opened this issue 1 year ago • 3 comments

I wanted to try Pigment CSS with Vite and this is the error I got. SCR-20241012-bajh

This has been broken with v6.0.0 release, so 6-7 weeks ago. I imagine the Pigment CSS team is responsible for this example.

I'm surprised I'm the first one to work on this. Like, how comes not even community's members faced this? It would mean that nobody uses this example, but then Vite is super popular, so it would mean nobody wants to try Pigment CSS which also doesn't make sense. I'm missing something. Maybe it's that +99% of the people are migrating applications, so nobody can use this example right away, only people who want to create bug reproductions use it today (until it's stable).


Side notes:

  • The example doesn't work with pnpm (it works with npm):
SCR-20241012-brja
  • I think I raised this in the past, I don't understand why @mui/material-pigment-css exists. I'm trying to think of a permutations that would explain it, but I can't connect the dots:

    • If it's because we need to sync breaking changes, we can pin versions, or replicate them between repos before releases. If Pigment CSS makes a breaking change, it should be replicated to Material UI instantly, users should adapt to it now, not later.
    • If it's because a wrapper allows smoothing the experience, I think it comes with so many downsides that I think it's clearly not worth it, one of the learnings of @mui/styles. For example, it doesn't force us to build the right extension theme tools like anyone else who would want to use Pigment CSS in their own product.
    • If it's because of the layout components, this is @mui/system / @mui/layout concern, not Pigment CSS concern. It should be a private implementation details
    • etc?

    How about we remove this package?

oliviertassinari avatar Oct 11 '24 22:10 oliviertassinari

Netlify deploy preview

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

Bundle size report

No bundle size changes (Toolpad) No bundle size changes

Generated by :no_entry_sign: dangerJS against 4bcf32bbb8db43c5fe8c52d300116af00aae40d1

mui-bot avatar Oct 11 '24 22:10 mui-bot

I think I raised this in the past, I don't understand why @mui/material-pigment-css exists. I'm trying to think of a permutations that would explain it, but I can't connect the dots:

  • If it's because we need to sync breaking changes, we can pin versions, or replicate them between repos before releases. If Pigment CSS makes a breaking change, it should be replicated to Material UI instantly, users should adapt to it now, not later.
  • If it's because a wrapper allows smoothing the experience, I think it comes with so many downsides that I think it's clearly not worth it, one of the learnings of @mui/styles. For example, it doesn't force us to build the right extension theme tools like anyone else who would want to use Pigment CSS in their own product.
  • If it's because of the layout components, this is @mui/system / @mui/layout concern, not Pigment CSS concern. It should be a private implementation details etc?

How about we remove this package?

The initial thought was that with v6, we want to ensure Material UI users that they are safe to try using Pigment CSS and there won't be breaking changes if they do. The only way to do this and at the same time allow us to iterate on the Pigment CSS API itself was with a wrapper package. However, since then, we realized that likely Pigment CSS is anyway not ready for production and started referring to it as an experimental API people can try. After this, honestly there is not much value in the wrapper package from this point of view.

However, if I remember correctly, we discussed with @brijeshb42 that we want to change some of the behavior/APIs that Pigment CSS support, but keep the old APIs for Mateiral UI users as they would otherwise have breaking changes. The only way to do this was with a wrapper package.

The way I see this in correlation with the previous setup is:

  • Emotion <-> PIgment CSS - different package that relates only to the styling engine
  • @mui/system <-> @mui/material-pigment-css - a wrapper package that adopts to the Material UI needs (likely we can improve the names)

The decision of keeping the package or removing is mostly related to what APIs we plan to have in @pigment-css/react and how close/far they are from the APIs we want to keep in Material UI.

I will let @brijeshb42 answer too, but these are my thoughts on the matter.

mnajdova avatar Oct 16 '24 07:10 mnajdova

+1 to what @mnajdova said. The wrapper package is only a means to an end and will probably be there till before v7 since the styled() API it supports right now will not be the same as the one in Pigment v1. So all the differences will be part of this package and it'll only cater to @mui/material integration. Currently, Pigment has a lot of Material UI baggage that should not be there.

brijeshb42 avatar Oct 16 '24 09:10 brijeshb42