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

[examples] Fix Pigment CSS Vite example #44074

Merged
merged 1 commit into from
Oct 22, 2024

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Oct 11, 2024

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 oliviertassinari added bug 🐛 Something doesn't work regression A bug, but worse examples Relating to /examples package: pigment-css Specific to @pigment-css/* labels Oct 11, 2024
@mui-bot
Copy link

mui-bot commented Oct 11, 2024

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 🚫 dangerJS against 4bcf32b

Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

I would assume that people that are trying this are migrating existing projects, this is why likely no one reported it.

@mnajdova
Copy link
Member

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.

@brijeshb42
Copy link
Contributor

+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.

@mnajdova mnajdova merged commit 3774a98 into mui:master Oct 22, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work examples Relating to /examples package: pigment-css Specific to @pigment-css/* regression A bug, but worse
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants