[utils] Add all @material-ui/core/utils to @material-ui/utils #23264

mnajdova · 2020-10-26T09:51:47Z

This PR prepares unnecessary changes for introducing the @material-ui/unstyled package. It moves the @material-ui/core/utils utilities to @material-ui/utils, so that those can be reused in the new package. All previous @material-ui/core/utils are now just re-exporting the @material-ui/utils

mui-pr-bot · 2020-10-26T09:54:37Z

@material-ui/utils: parsed: +71.54% , gzip: +65.22%

Details of bundle changes

Generated by 🚫 dangerJS against 1312f19

mnajdova · 2020-10-26T10:18:20Z

packages/material-ui-utils/src/capitalize.ts

@@ -0,0 +1,13 @@
+// TODO: error TS7016: Could not find a declaration file for module '../macros/MuiError.macro'. '/tmp/material-ui/packages/material-ui-utils/macros/MuiError.macro.js' implicitly has an 'any' type.
+// import MuiError from '../macros/MuiError.macro';


@eps1lon FYI we have this issue with the macros

Could you @ts-expect-error this instead for now so that we don't sacrifice the runtime behavior for typings?

oliviertassinari · 2020-10-26T12:30:35Z

packages/material-ui-utils/src/index.ts

+export { default as requirePropFactory } from './requirePropFactory';
+export { default as setRef } from './setRef';
+export { default as unstable_useEnhancedEffect } from './useEnhancedEffect';
+export { default as unstable_useId } from './useId';


Do we need the unstable prefix if we treat all the modules here private?

If they're here, they're public.

I forgot where I said but I would be way more comfortable by renaming /utils to internal or even unsafe_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.

Should we then proceed with #23270 move all utils in unstyled and export them as unstable_? Then for avoiding breaking changes, we can reuse them in the core and just reexport them.

Then for avoiding breaking changes, we can reuse them in the core and just reexport them.

I don't understand what that solves.

If we change the API of useId and only we consume useId then everything is fine.
If useId can also be used by library consumers directly then we can only release a breaking change either by

bumping the major version number of the package

bumping the pre-release number if it's an alpha

do nothing (but documentation) if it's prefixed as unstable_
unstable_ is still only an informal convention so we should still be careful.

or should we move them to the new@material-ui/unstyled package and again to export them as unstable_

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.

Should we move all these utils to @material-ui/utils as this PR is doing and export them all as unstable_

I think that this option can work 👍.

Alright, let me update the exports then

Done, everything in @material-ui/utils that comes from @material-ui/core/utils is exported as unstable_

We're importing unstable_createFunction from /utils and re-export it as createChainedFunction 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?

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.

packages/material-ui-utils/src/capitalize.ts

eps1lon · 2020-10-26T12:36:41Z

packages/material-ui/src/utils/ownerDocument.ts

-}
+import { ownerDocument } from '@material-ui/utils';
+
+export default ownerDocument;


nit: I think we should remove these files. As far as I can tell they're not necessary anymore which means we can safely reduce the number of files we ship.

We have local imports in all core components to this files, I am re-exporting to avoid changes in all core files. I've started with that approach here #23203 but there were too many changes

Sounds good. Let's keep this conversation open for visibility.

…at-self Add compat for usage in utils itself

oliviertassinari

I love the direction taken. I think that it's a great compromise.

Issue with the macro was resolved

eps1lon · 2020-10-28T09:57:26Z

packages/material-ui-utils/src/useId.ts

+import * as React from 'react';
+
+/**
+ * Private module reserved for @material-ui packages.


Could we resolve what the intent of @material-ui/utils is? This is exported as unstable_ but annotated as private. The current state of @material-ui/utils is a mixed bag that can not be understood reasonably by npm users and it looks like even we don't know what this package is.

We'd do us a big favor if we would just rename it to /internal so we don't have to worry about maintaining utils and then we bump the required version in the dependent packages on every release.

It's not a blocker for this PR but definitely for the next release.

@eps1lon Agree! Let's try to come up with an answer. You are right unstable mean we plan to make it public while private, well, is private. However, I don't think that we should solve it here. For later.

Maybe the following?

@material-ui/utils -> @material-ui/internal

@material-ui/types -> @material-ui/internal

@material-ui/unstyled depends on @material-ui/internal

@material-ui/system depends on @material-ui/internal

@material-ui/styles depends on @material-ui/internal

@material-ui/unstyled reexport some of the utils we want to make public

@material-ui/core reexport some of the utils we want to make public, from @material-ui/unstyled

@material-ui/internal is versioned as alpha so we can make BCs

@eps1lon I have noticed @material-ui/types used in the documentation for one use case:

https://github.com/mui-org/material-ui/blob/0aefc928b00788b0ec5f74842b7c9704a354cf4a/docs/src/pages/components/breadcrumbs/RouterBreadcrumbs.tsx#L15

do we still need it with the new versions of TypeScript?

do we still need it with the new versions of TypeScript?

We don't need it for new versions of typescript but haven't had any discussions about bumping the required versions.

reexport some of the utils we want to make public

Then what is the point of moving them into an internal package if we just re-export them? That's not what internal means.

haven't had any discussions about bumping the required versions.

@eps1lon Do we need a discussion about changing the supported version of TypeScript?

Then what is the point of moving them into an internal package if we just re-export them

Great question, the only value I can think of is to allow @material-ui/system to depend on @material-ui/internal rather than @material-ui/unstyled (also true for @material-ui/styles but the packages it getting deprecated so I don't think that we should optimize for it).

Do we need a discussion about changing the supported version of TypeScript?

Like any other breaking change: yes.

Like any other breaking change: yes.

Sorry, I wasn't clear. Do we want/need to change the minimum version of TypeScript supported? I'm personally not familiar with the matter.

I haven't though about it but since we didn't change anything we still need an Omit polyfill.

It's just that we test with the latest version where we don't need it. But 3.2 does which is what we support.

eps1lon · 2020-10-28T10:02:52Z

Also: can we make the TS migration in another PR? We now have .d.ts files alongside .ts files and some .ts files just have a big @ts-ignore making it meaningless that they're written in .ts.

mnajdova · 2020-10-28T10:13:45Z

Also: can we make the TS migration in another PR? We now have .d.ts files alongside .ts files and some .ts files just have a big @ts-ignore making it meaningless that they're written in .ts.

You mean to move the same .js & d.ts files we had before from the @material-ui/utils package?

mnajdova · 2020-10-28T10:17:36Z

Also: can we make the TS migration in another PR? We now have .d.ts files alongside .ts files and some .ts files just have a big @ts-ignore making it meaningless that they're written in .ts.

That will probably mean we need to change the build of that package, because as far as I remember it was ts module before.

eps1lon · 2020-10-28T10:18:58Z

You mean to move the same .js & d.ts files we had before from the @material-ui/utils package?

I thought we move from /core/utils to /utils so I meant keep the files as-is from /core/utils.

mnajdova · 2020-10-28T11:30:53Z

I thought we move from /core/utils to /utils so I meant keep the files as-is from /core/utils.

Done, the only thing missing was a definition file for scrollLeft.js, which is added in this PR, all other files are moved as is.

mbrookes

Mostly just a bunch of trivial changes to internal comments

packages/material-ui-utils/macros/__fixtures__/relative-import/error-codes.json

packages/material-ui-utils/macros/__fixtures__/relative-import/input.js

packages/material-ui-utils/macros/__fixtures__/relative-import/output.js

packages/material-ui-utils/src/capitalize.js

packages/material-ui-utils/src/useIsFocusVisible.js

packages/material-ui-utils/src/isMuiElement.d.ts

packages/material-ui-utils/src/useId.js

…/error-codes.json Co-authored-by: Matt <github@nospam.33m.co>

…/input.js Co-authored-by: Matt <github@nospam.33m.co>

…/output.js Co-authored-by: Matt <github@nospam.33m.co>

Co-authored-by: Matt <github@nospam.33m.co>

packages/material-ui-utils/src/createChainedFunction.js

king-of-poppk · 2024-08-06T13:34:05Z

packages/material-ui-utils/src/debounce.js

+  let timeout;
+  function debounced(...args) {
+    const later = () => {
+      func.apply(this, args);


Any reason why we do not timeout = undefined here? AFAICT, the setTimeout return value is a number in browsers, and these could be recycled.

Happy to submit a PR if this makes sense.

move core/utils to utils

5901bda

mnajdova requested a review from oliviertassinari October 26, 2020 09:51

mnajdova mentioned this pull request Oct 26, 2020

[utils] Move @material-ui/core/utils to @material-ui/utils #23203

Closed

prettier

c9489d7

mnajdova commented Oct 26, 2020

View reviewed changes

mnajdova requested a review from eps1lon October 26, 2020 10:18

oliviertassinari approved these changes Oct 26, 2020

View reviewed changes

oliviertassinari added the core Infrastructure work going on behind the scenes label Oct 26, 2020

eps1lon previously requested changes Oct 26, 2020

View reviewed changes

mnajdova added 2 commits October 26, 2020 13:49

MuiError

e866f5e

ts-ignore

080dd7e

mnajdova mentioned this pull request Oct 26, 2020

[types] Moved types from OverridableComponent.d.ts to @material-ui/types #23265

Closed

github-actions bot mentioned this pull request Oct 26, 2020

Update snapshots for pr/23264 eps1lon/mui-scripts-incubator#1213

Closed

mnajdova mentioned this pull request Oct 26, 2020

[unstyled] Create package and move SliderUnstyled there #23270

Merged

eps1lon and others added 4 commits October 27, 2020 11:21

Add compat for usage in utils itself

c625415

Fix lint

bd907da

exported new utils as unstable_

324b3a8

Merge pull request #11 from eps1lon/feat/add-core-utils-to-utils-comp…

11bbc63

…at-self Add compat for usage in utils itself

mnajdova requested review from eps1lon and mbrookes October 27, 2020 15:37

oliviertassinari approved these changes Oct 27, 2020

View reviewed changes

eps1lon reviewed Oct 28, 2020

View reviewed changes

mnajdova added 2 commits October 28, 2020 11:53

replaced .ts files with original .js & d.ts files

a9a4247

fixed import

3957119

mnajdova requested a review from eps1lon October 28, 2020 11:53

eps1lon approved these changes Oct 28, 2020

View reviewed changes

mbrookes reviewed Oct 28, 2020

View reviewed changes

mnajdova and others added 22 commits October 28, 2020 19:20

Update packages/material-ui-utils/macros/__fixtures__/relative-import…

9066f7a

…/error-codes.json Co-authored-by: Matt <github@nospam.33m.co>

Update packages/material-ui-utils/macros/__fixtures__/relative-import…

e0f137c

…/input.js Co-authored-by: Matt <github@nospam.33m.co>

Update packages/material-ui-utils/macros/__fixtures__/relative-import…

2d1ac56

…/output.js Co-authored-by: Matt <github@nospam.33m.co>

Update packages/material-ui-utils/src/capitalize.js

c9e7ba0

Co-authored-by: Matt <github@nospam.33m.co>

Update packages/material-ui-utils/src/capitalize.js

7a3b4c2

Co-authored-by: Matt <github@nospam.33m.co>

Update packages/material-ui-utils/src/createChainedFunction.js

fca1e8d

Co-authored-by: Matt <github@nospam.33m.co>

Update packages/material-ui-utils/src/setRef.ts

ebfdc06

Co-authored-by: Matt <github@nospam.33m.co>

Update packages/material-ui-utils/src/setRef.ts

623a89e

Co-authored-by: Matt <github@nospam.33m.co>

Update packages/material-ui-utils/src/setRef.ts

76810d7

Co-authored-by: Matt <github@nospam.33m.co>

Update packages/material-ui-utils/src/setRef.ts

831d74b

Co-authored-by: Matt <github@nospam.33m.co>

Update packages/material-ui-utils/src/setRef.ts

e0bc921

Co-authored-by: Matt <github@nospam.33m.co>

Update packages/material-ui-utils/src/useControlled.d.ts

b5fe610

Co-authored-by: Matt <github@nospam.33m.co>

Update packages/material-ui-utils/src/useForkRef.js

39dac58

Co-authored-by: Matt <github@nospam.33m.co>

Update packages/material-ui-utils/src/useControlled.js

b65508e

Co-authored-by: Matt <github@nospam.33m.co>

Update packages/material-ui-utils/src/useIsFocusVisible.js

6fd966f

Co-authored-by: Matt <github@nospam.33m.co>

Update packages/material-ui-utils/src/useIsFocusVisible.js

837e33f

Co-authored-by: Matt <github@nospam.33m.co>

Update packages/material-ui-utils/src/useIsFocusVisible.js

9eefcca

Co-authored-by: Matt <github@nospam.33m.co>

Update packages/material-ui-utils/src/useIsFocusVisible.js

5706036

Co-authored-by: Matt <github@nospam.33m.co>

Update packages/material-ui-utils/src/useIsFocusVisible.js

21c706d

Co-authored-by: Matt <github@nospam.33m.co>

Removed private method comment

18be046

removed private method comment

8438658

remove private method comment

e2104a5

mnajdova commented Oct 28, 2020

View reviewed changes

packages/material-ui-utils/src/createChainedFunction.js Outdated Show resolved Hide resolved

Update packages/material-ui-utils/src/createChainedFunction.js

1312f19

mnajdova merged commit 03bd73b into mui:next Oct 28, 2020

king-of-poppk reviewed Aug 6, 2024

View reviewed changes

		@@ -0,0 +1,13 @@
		// TODO: error TS7016: Could not find a declaration file for module '../macros/MuiError.macro'. '/tmp/material-ui/packages/material-ui-utils/macros/MuiError.macro.js' implicitly has an 'any' type.
		// import MuiError from '../macros/MuiError.macro';

[utils] Add all @material-ui/core/utils to @material-ui/utils #23264

[utils] Add all @material-ui/core/utils to @material-ui/utils #23264

Conversation

mnajdova commented Oct 26, 2020

mui-pr-bot commented Oct 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari Oct 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnajdova Oct 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari Oct 28, 2020 • edited Loading

Choose a reason for hiding this comment

oliviertassinari Oct 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari Oct 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari Oct 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eps1lon commented Oct 28, 2020

mnajdova commented Oct 28, 2020

mnajdova commented Oct 28, 2020

eps1lon commented Oct 28, 2020

mnajdova commented Oct 28, 2020

mbrookes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mui-pr-bot commented Oct 26, 2020 •

edited

Loading

oliviertassinari Oct 27, 2020 •

edited

Loading

mnajdova Oct 27, 2020 •

edited

Loading

oliviertassinari left a comment •

edited

Loading

oliviertassinari Oct 28, 2020 •

edited

Loading

oliviertassinari Oct 28, 2020 •

edited

Loading

oliviertassinari Oct 28, 2020 •

edited

Loading

oliviertassinari Oct 28, 2020 •

edited

Loading