-
-
Notifications
You must be signed in to change notification settings - Fork 47
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
Conversation
Netlify deploy preview |
import * as React from 'react'; | ||
import { expect } from 'chai'; | ||
import { act, createRenderer } from '@mui/internal-test-utils'; | ||
import { useControlled } from './useControlled'; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 ${ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe?
`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?
`MUI: A component is changing the ${ | |
`A component is changing the ${ |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I like it. I wouldn't do this in this PR, though. I'll change the prefix to Base UI here, since for now only we use these utils, and create a follow-up one to configure it.
93c78f7
to
ff29cc2
Compare
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).