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

Draft: [PopperUnstyled] Replace Popper.js package with Floating UI package #35914

Closed
wants to merge 4 commits into from

Conversation

hbjORbj
Copy link
Member

@hbjORbj hbjORbj commented Jan 23, 2023

Ref - Migration from Popper 2 to Floating UI: https://floating-ui.com/docs/migration

@mui-bot
Copy link

mui-bot commented Jan 23, 2023

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

Bundle size will be reported once CircleCI build #475116 finishes.

Generated by 🚫 dangerJS against 592cda3

@hbjORbj hbjORbj added component: Popper The React component. See <Popup> for the latest version. package: base-ui Specific to @mui/base labels Jan 23, 2023
@hbjORbj hbjORbj force-pushed the menu-unstyled/floating-ui branch from c88ee26 to 5c5fb0a Compare January 23, 2023 07:08
@hbjORbj hbjORbj self-assigned this Jan 23, 2023
@michaldudak
Copy link
Member

Have you tried using Floating UI directly in our components, without wrapping it in the PopperUnstyled component?

@oliviertassinari oliviertassinari marked this pull request as draft January 28, 2023 15:04
@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 28, 2023

Have you tried using Floating UI directly in our components, without wrapping it in the PopperUnstyled component?

@michaldudak Do you mean using the explosed React API and not the exposed DOM API?

I think the ideal world would be: we have no dependencies. So at first sight, I would explore the other direction: how can we expose less API surface? This could bring:

@michaldudak
Copy link
Member

Do you mean using the exposed React API and not the exposed DOM API?

Yes, that's what I meant.

I think the ideal world would be: we have no dependencies

Why? If there's a library that fits our needs well, is maintained, doesn't add significant weight to the bundle size, and is tree-shakable, why would we reinvent the wheel, create and maintain something on our own?
As for the API, we could expose a higher-level utility, such as Popover. Floating UI could be just our internal utility.

@oliviertassinari
Copy link
Member

oliviertassinari commented Mar 2, 2023

As for the API, we could expose a higher-level utility, such as Popover. Floating UI could be just our internal utility.

@michaldudak I think that this is aligned with what seems to make sense long-term with the CSS implementation: 👍.

Why? If there's a library that fits our needs well, is maintained, doesn't add significant weight to the bundle size, and is tree-shakable, why would we reinvent the wheel, create and maintain something on our own?

If it's an internal dependency, the value to consider is about simplifying the supply chain, to reduce the failure modes that are outside of our control: breaking change release, security issues, license mismatch, extra npm package size, browser support. For example, Next.js inlines all Babel in their npm package (hiding the dependency).

If it's a user-facing dependency, then I think that it's about whether we see an opportunity to improve the situation for the users or not.

@atomiks
Copy link
Contributor

atomiks commented Mar 18, 2023

@oliviertassinari the best thing to do on MUI's side is not to expose internal details about Floating UI, such as configuration options, etc. Wrapping it in an API that's close to the CSS anchoring spec is good for future-proofing 👍 this would allow you to upgrade majors more easily if needed (we're not doing any fundamental API changes again though, only small things from now on. And there's no plans for v2 any time soon).

@hbjORbj
Copy link
Member Author

hbjORbj commented Mar 23, 2023

@atomiks Hey, I tried again with the latest version of Floating UI (I used @floating-ui/react as it's a superset) to test, but I still get this weird build error:

error - ../packages/mui-icons-material/lib/ArrowDropDownRounded.js (10:0) @ eval
error - TypeError: (0 , _createSvgIcon.default) is not a function
    at eval (webpack-internal:///../packages/mui-icons-material/lib/ArrowDropDownRounded.js:10:43)

I left this PR as a draft because I couldn't figure out the build error. Do you have a clue where this might be coming from? You can reproduce the error by installing any FloatingUI package in MUI codebase and running the docs (yarn docs:dev).

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 11, 2023
@ZeeshanTamboli
Copy link
Member

Closing this PR since a new component (Popup) is being developed which will use Floating UI behind the scenes - #37960.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: Popper The React component. See <Popup> for the latest version. package: base-ui Specific to @mui/base PR: out-of-date The pull request has merge conflicts and can't be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants