Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[utils] Add all @material-ui/core/utils to @material-ui/utils #23264
[utils] Add all @material-ui/core/utils to @material-ui/utils #23264
Changes from 34 commits
5901bda
c9489d7
e866f5e
080dd7e
c625415
bd907da
324b3a8
11bbc63
a9a4247
3957119
328ca77
2747327
9066f7a
e0f137c
2d1ac56
c9e7ba0
7a3b4c2
fca1e8d
ebfdc06
623a89e
76810d7
831d74b
e0bc921
b5fe610
39dac58
b65508e
6fd966f
837e33f
9eefcca
5706036
21c706d
18be046
8438658
e2104a5
1312f19
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Any reason why we do not
timeout = undefined
here? AFAICT, thesetTimeout
return value is anumber
in browsers, and these could be recycled.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.
Happy to submit a PR if this makes sense.
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.
Do we need the unstable prefix if we treat all the modules here private?
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.
If they're here, they're public.
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 forgot where I said but I would be way more comfortable by renaming
/utils
tointernal
or evenunsafe_internal
. There are already plenty of packages depending on/types
and I suspect the same happened with/utils
.Or we release it as an alpha.
Either way, I agree that the current state of /utils being on the same release line as /core (5.x) does not signal that the package is private. It's simply not enough that we treat it as private since we have some responsibility to not clutter npm with more underdocumented packages.
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.
Should we then proceed with #23270 move all utils in
unstyled
and export them asunstable_
? Then for avoiding breaking changes, we can reuse them in the core and just reexport them.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 don't understand what that solves.
If we change the API of
useId
and only we consumeuseId
then everything is fine.If
useId
can also be used by library consumers directly then we can only release a breaking change either byunstable_
unstable_
is still only an informal convention so we should still be careful.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.
Considering that some of the utils are required by the system, it would mean the system needs to depend on unstyled. I assume that it's not something we want because the two are solving independent problems.
I think that this option can work 👍.
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.
Alright, let me update the exports then
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.
Done, everything in
@material-ui/utils
that comes from@material-ui/core/utils
is exported asunstable_
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.
We're importing
unstable_createFunction
from /utils and re-export it ascreateChainedFunction
from /core/utils. I don't know how this is supposed to work: If /utils can change the API of createChainedFunction on every release and /core pins /utils to major then createChainedFunction from /core/utils can also change on every release and is therefore unstable.What did we try to accomplish by adding (and not moving) /core/utils to /utils?
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 don't want to make changes to the existing
core/utils
as we have imports all over the project from there. I started like that in #23203 prepared codemods as well, but we decided to go wtih the minimal changes necessary for unblocking ourselves, which is to move the utils to core, but still re-export them from the core in order to avoid changes in the core.