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

[core] Remove @mui/utils and @mui/types dependencies #827

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

michaldudak
Copy link
Member

@michaldudak michaldudak commented Nov 18, 2024

Moved utilities to our codebase so we don't depend on more libraries than necessary. In the future, Core projects will use utilities from Base UI (mui/material-ui#44471).

@michaldudak michaldudak added core Infrastructure work going on behind the scenes dependencies Update of dependencies labels Nov 18, 2024
@mui-bot
Copy link

mui-bot commented Nov 18, 2024

Netlify deploy preview

https://deploy-preview-827--base-ui.netlify.app/

Generated by 🚫 dangerJS against a44a8e0

packages/mui-base/src/utils/useId.test.tsx Outdated Show resolved Hide resolved
import * as React from 'react';
import { expect } from 'chai';
import { act, createRenderer } from '@mui/internal-test-utils';
import { useControlled } from './useControlled';
Copy link
Member

Choose a reason for hiding this comment

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

What if we were to already expose them as a new npm package https://www.notion.so/mui-org/engineering-mui-utils-purpose-9a9fc9da3a004864b6c4e1f4d1f24f95?p=56e98feced4d4fe4a73df99e0b0361b4&pm=s while we are at it? so that Material UI, MUI X, can migrate to those and @mui/utils can deprecate those utils?

Or maybe it's too big of a jump at once?

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's do it incrementally. As I noted in https://www.notion.so/engineering-mui-utils-purpose-9a9fc9da3a004864b6c4e1f4d1f24f95?d=143cbfe7b66080e1ad16001c70691469&pvs=4#7cb73e22aaee40eb9c35cca853ffc73f, I'm not particularly keen on placing this kind of utilities in a separate package. I think they are useful to build styled libraries and should be exposed from @base-ui-components/react directly.

if (isControlled !== (controlled !== undefined)) {
console.error(
[
`MUI: A component is changing the ${
Copy link
Member

Choose a reason for hiding this comment

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

Maybe?

Suggested change
`MUI: A component is changing the ${
`Base UI: A component is changing the ${

or we could remove this, and hope that facebook/react#28992 (comment) will get fixed?

Suggested change
`MUI: A component is changing the ${
`A component is changing the ${

Copy link
Member Author

Choose a reason for hiding this comment

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

It's an interesting problem. If this is imported in Material UI, users might get confused where does this "Base UI" come from. But it's equally true for all the other error messages (such as missing context providers). We might be better off removing the prefix in all cases. CC @atomiks

Copy link
Contributor

Choose a reason for hiding this comment

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

It could be possible to configure the prefix name at runtime before any messages are logged

import {log} from '@base-ui-components/react';

log.prefix('Base UI: ');

Then we could use utilities to throw errors:

log.error('...'); // throws, prefixed
log.warn('...'); // dedupes, prefixed

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes dependencies Update of dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants